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

fix(cascadingfilterprocessor): prevent overriding metrics in cascading filter processor - add processor tag #539

Conversation

dmolenda-sumo
Copy link
Contributor

When two cascading filter processor instances were running on one collector, the metrics values were overridden. Adding the instance_name tag fixes this issue

@dmolenda-sumo dmolenda-sumo requested a review from a team as a code owner April 8, 2022 10:16
@github-actions github-actions bot added the go label Apr 8, 2022
@sumo-drosiek
Copy link
Contributor

I think it should be named processor

@sumo-drosiek
Copy link
Contributor

Also please rename all Instance variables to Processor

@dmolenda-sumo dmolenda-sumo force-pushed the dmolenda-add-instance-name-tag-to-cascading-filter-processor-metrics branch from cc9af28 to b1c381b Compare April 8, 2022 13:45
@sumo-drosiek
Copy link
Contributor

Please use conventional commits (at least as PR name, so it will be used while squashing and merging) and also please make Ci to be happy 🤗

@swiatekm
Copy link

swiatekm commented Apr 8, 2022

Please use conventional commits (at least as PR name, so it will be used while squashing and merging) and also please make Ci to be happy 🤗

We should just enable https://github.com/zeke/semantic-pull-requests in this repo, just as we have in the Helm chart one.

@dmolenda-sumo dmolenda-sumo changed the title Add instance_name tag to metrics in cascading filter processor fix(cascadingfilterprocessor): prevent overriding metrics in cascading filter processor - add processor tag Apr 11, 2022
Copy link

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

Two requests:

  1. I think this should have a changelog entry.
  2. Can you squash your commits into one?

LGTM otherwise.

@dmolenda-sumo dmolenda-sumo force-pushed the dmolenda-add-instance-name-tag-to-cascading-filter-processor-metrics branch from 5c710ab to 665bb07 Compare April 13, 2022 13:00
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Apr 13, 2022
@dmolenda-sumo dmolenda-sumo force-pushed the dmolenda-add-instance-name-tag-to-cascading-filter-processor-metrics branch from 665bb07 to 71e2df0 Compare April 13, 2022 13:02
CHANGELOG.md Outdated Show resolved Hide resolved
pkg/processor/cascadingfilterprocessor/processor.go Outdated Show resolved Hide resolved
pkg/processor/cascadingfilterprocessor/processor.go Outdated Show resolved Hide resolved
@dmolenda-sumo dmolenda-sumo force-pushed the dmolenda-add-instance-name-tag-to-cascading-filter-processor-metrics branch from 71e2df0 to cea6353 Compare April 19, 2022 14:19
CHANGELOG.md Outdated
@@ -14,10 +14,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Fixed

- fix(sumologicexporter): treat resource attributes as fields for otlp #536
- fix(cascadingfilterprocessor): prevent overriding metrics in cascading filter processor - add processor tag #539
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 want to keep this type of links (i.e. #539 which wouldn't work outside of Github vs embedded markdown links [#539][#539]) ?

@SumoLogic/open-source-collection-team ?

Choose a reason for hiding this comment

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

I'd personally prefer normal MD links that will work locally as well.

@dmolenda-sumo
Copy link
Contributor Author

@SumoLogic/open-source-collection-team Do you have any idea what is wrong with the check: PRs checks / markdown-link-check (pull_request)? How to fix this?

@pmalek-sumo
Copy link
Contributor

@SumoLogic/open-source-collection-team Do you have any idea what is wrong with the check: PRs checks / markdown-link-check (pull_request)? How to fix this?

It seems its a known issue with the latest version: gaurav-nelson/github-action-markdown-link-check#127

Let's try to rerun it, it should work just fine as v1 was pinned to previous working version (1.0.13 instead of broken 1.0.14)

@swiatekm
Copy link

I reran the link check and it passed. @dmolenda-sumo you just need to rebase on main and we should be good to merge.

@dmolenda-sumo dmolenda-sumo force-pushed the dmolenda-add-instance-name-tag-to-cascading-filter-processor-metrics branch from 54b0bf8 to c877137 Compare April 25, 2022 09:58
@dmolenda-sumo dmolenda-sumo merged commit 4e15a72 into main Apr 25, 2022
@dmolenda-sumo dmolenda-sumo deleted the dmolenda-add-instance-name-tag-to-cascading-filter-processor-metrics branch April 25, 2022 10:14
kasia-kujawa pushed a commit that referenced this pull request May 13, 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.

4 participants