Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(sumologicexporter): remove setting source headers #686

Merged

Conversation

aboguszewski-sumo
Copy link
Contributor

Resolves #255
Ref: #685

This PR removes the functionality of setting source headers in the sumologic exporter, which has been deprecated some time ago.

@aboguszewski-sumo aboguszewski-sumo added this to the v0.60 milestone Aug 1, 2022
@github-actions github-actions bot added documentation Improvements or additions to documentation go labels Aug 1, 2022
@aboguszewski-sumo aboguszewski-sumo force-pushed the source-templates-breaking branch 2 times, most recently from 5e20734 to 6200011 Compare August 2, 2022 09:17
@aboguszewski-sumo aboguszewski-sumo marked this pull request as ready for review September 23, 2022 12:48
@aboguszewski-sumo aboguszewski-sumo requested a review from a team as a code owner September 23, 2022 12:48
Comment on lines 135 to 140
if cfg.SourceCategory != "" || cfg.SourceHost != "" || cfg.SourceName != "" {
return fmt.Errorf(`*Deprecation warning*: setting source headers is not supported anymore.
Please consult the changelog at https://github.com/SumoLogic/sumologic-otel-collector/releases/tag/v0.60.0-sumo-0`,
)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we really need it. I found some time ago that such warnings have been printed so I added it.
It was also added for attribute translation and telegraf metrics translation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We added that because it was deprecated, now it's going to be removed :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first such warning was this:

if len(cfg.MetadataAttributes) > 0 {
return fmt.Errorf(`*Deprecation warning*: metadata_attributes is not supported anymore.
Please consult the changelog at https://github.com/SumoLogic/sumologic-otel-collector/releases/tag/v0.49.0-sumo-0`,
)
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ok, but it probably shouldn't say deprecation warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sumo-drosiek @swiatekm-sumo what's the conclusion then? I'm not against any of the options, but if we don't want it here, we should probably also remove these warnings for other deleted options.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd leave the error message, just without calling it a deprecation. WDYT @sumo-drosiek ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@swiatekm-sumo we can do that

@aboguszewski-sumo aboguszewski-sumo force-pushed the source-templates-breaking branch from c100e4a to 4189d4a Compare October 18, 2022 09:10
@aboguszewski-sumo aboguszewski-sumo enabled auto-merge (rebase) October 18, 2022 09:10
@aboguszewski-sumo aboguszewski-sumo merged commit 083337c into SumoLogic:main Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation go
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[sourceprocessor,sumologicexporter] Remove source templates
3 participants