-
Notifications
You must be signed in to change notification settings - Fork 183
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
Update OTC to v0.38.1-sumo #1893
Conversation
Run some tests and overall it looks good |
spans_per_second: -1 | ||
probabilistic_filtering_ratio: 0.2 | ||
spans_per_second: 1660 | ||
num_traces: 200000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That changes the default, doesn't it? Is that ok for the users? Are we planning to document this in the CHANGELOG.md/release notes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a concern since we never explained how to use prefilled values in values.yaml
for cascading_filter
. The initial idea was to provide instructions on how to just enable the processor in the pipeline, but this was never done and eventually we provide a template with instructions instead. So practically this might be considered a new thing.
I am pretty sure we want to mention OTC update in the changelog though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree on including it in the Changelog.
I understand the explanation but this still in some way is a breaking change 😓 for our users. It was possible to have this configured in the old yaml "schema" and now it would change with this PR being merged, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it won't change much but of course we can mention this as well. So essentially something like:
- Update OpenTelemetry Collector version to v0.38.1-sumo
- Move
insecure
parameter to separate configuration variable - Fix OTLP/HTTP metadata tagging
- Update Cascading Filter to new version which adds filtering by number of errors and uses new config format
- Change the default number of traces for Cascading Filter to 200000
- Move
@mat-rumian what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you could add an entry about this change in the changelog that would be great 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the changelog and put some links there
f56de3b
to
3eda5ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
@pmm-sumo Please update the description and we can merge it afterwards. |
Description
Update OpenTelemetry Collector version to v0.38.1-sumo
by number of errors and switches to a new, easier to use config format
Testing performed