Cleanup static mode after the otel upgrade, fix bug with Jaeger remote sampling #4640
Conversation
ptodev
changed the title
ptodev
force-pushed
the
static-mode-cleanup-after-otel-upgrade
branch
from
9d6ffd8
to
8299cdb
Compare
docs/sources/flow/upgrade-guide.md
Outdated
| ### Breaking change: Jaeger remote sampling does not work | ||
|
|
||
| Jaeger remote sampling used to be configured via the jaeger receiver. This receiver was updated to a new version, and remote sampling has stopped working. | ||
|
|
||
| Jaeger remote sampling will be reintroduced in v35.3. It will be configured separately from the jaeger receiver. |
There was a problem hiding this comment.
I don't know if we should introduce new functionality in a patch release; it goes against semver. We could say this is a known issue in v0.35 and to wait until v0.36 for a fix.
There was a problem hiding this comment.
If it is something that is currently broken then fixing it in a patch via new functionality feels reasonable. The other option that is most semver compatible is to roll back the otel upgrade. Given the effort there that is painful to do. @ptodev have we gotten any reports on this or did we find it?
I guess fundamentally though we could roll out a v0.36 instead of a patch if we wanted to align with semver?
There was a problem hiding this comment.
Yeah I'm not going to push too hard on this, but it does feel bad to categorize it as a breaking change, since that feels more deliberate/planned, and this is just an artifact of upgrading.
There was a problem hiding this comment.
Agreed, this feels like a breaking change that became a bug.
There was a problem hiding this comment.
have we gotten any reports on this or did we find it?
It was reported internally by another team in Grafana.
it does feel bad to categorize it as a breaking change, since that feels more deliberate/planned, and this is just an artifact of upgrading
I agree. The reason I listed it as a breaking change is to raise awareness in case people are yet to upgrade, or have upgraded and experienced issues. I could clarify that this was an unintentional breaking change, a bug, and the bug will be fixed in v35.3? I don't think it goes against semver, since I really think of this as a bugfix rather than a new feature.
Co-authored-by: Robert Fratto <robertfratto@gmail.com>
ptodev
force-pushed
the
static-mode-cleanup-after-otel-upgrade
branch
from
026d419
to
492a016
Compare
|
Thank you for the reviews. I updated the doc with Robert's suggestions, added example configs to the upgrade guide, and also moved my changes from the flow upgrade guide to the static one (I didn't notice it earlier, apologies 🤦♂️ ). I'll merge this when the CI finishes. |
…e sampling (#4640) * Cleanup static mode after the otel upgrade * Fix linter issues * Update docs/sources/flow/upgrade-guide.md Co-authored-by: Robert Fratto <robertfratto@gmail.com> * Move breaking change note to static mode doc, add example configs --------- Co-authored-by: Robert Fratto <robertfratto@gmail.com>
…e sampling (#4640) * Cleanup static mode after the otel upgrade * Fix linter issues * Update docs/sources/flow/upgrade-guide.md Co-authored-by: Robert Fratto <robertfratto@gmail.com> * Move breaking change note to static mode doc, add example configs --------- Co-authored-by: Robert Fratto <robertfratto@gmail.com>
…e sampling (#4640) * Cleanup static mode after the otel upgrade * Fix linter issues * Update docs/sources/flow/upgrade-guide.md Co-authored-by: Robert Fratto <robertfratto@gmail.com> * Move breaking change note to static mode doc, add example configs --------- Co-authored-by: Robert Fratto <robertfratto@gmail.com>
github-actions
bot
added
the
frozen-due-to-age
PR Description
There is an issue with Jaeger remote sampling not working after #3858 was merged. This PR will fix it.
I also updated the docs, and updated other components in the Static mode's tracing subsystem to use new parameters which were not previously available on the old Otel version.
Notes to the Reviewer
main.jaeger_remote_sampling:in the config. However, I could not get that to work.PR Checklist