-
Notifications
You must be signed in to change notification settings - Fork 250
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
otelcol: decouple otel/alloy component IDs #882
Conversation
Signed-off-by: Paschalis Tsilias <[email protected]>
// Inform listeners that our handler changed. | ||
a.opts.OnStateChange(Exports{ | ||
Handler: Handler{ | ||
ID: otelcomponent.NewID(otelcomponent.MustNewType(cTypeStr)), | ||
ID: otelcomponent.NewID(otelcomponent.MustNewType(typeStr)), |
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.
This would be a collision issue if there's ever two auth extensions used for a component; do you know if that's possible? Even if it's not, should we preemptively defend against that?
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.
AFAICT the only way to use auth components is currently in exporters which can only ever instantiate one client
block. As far as I've read the code and tested, it shoulnd't be an issue.
I see the point though to make sure we don't trip over ourselves in the future.
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.
PTAL. Just for being defensive, I'm using a small hash when the 63-char limit is exceeded, in a helper function. This can likely move to a shared place if we need it again.
Signed-off-by: Paschalis Tsilias <[email protected]>
Based on this:
It does sound like a user-facing change, and so probably needs a changelog entry, since the user could provide a component label which is too long and causes the function to throw a panic (IIUC). |
Signed-off-by: Paschalis Tsilias <[email protected]>
Signed-off-by: Paschalis Tsilias <[email protected]> (cherry picked from commit d018e6e)
* Fix panic when component ID contains `/` in `otelcomponent.MustNewType(ID)` (#858) Signed-off-by: Weifeng Wang <[email protected]> (cherry picked from commit 7bae89c) * No error when http fails (#841) * Fail if we see the port is not available * Update changelog * cleanup message * Update CHANGELOG.md Co-authored-by: Erik Baranowski <[email protected]> --------- Co-authored-by: Erik Baranowski <[email protected]> (cherry picked from commit 4ca3f84) * fix panic loki source docker (#875) (cherry picked from commit 4fb1df9) * clustering: fix ipv6 advertise addresses (#869) Signed-off-by: Matthew Penner <[email protected]> (cherry picked from commit 3df2cd0) * otelcol: decouple otel/alloy component IDs (#882) Signed-off-by: Paschalis Tsilias <[email protected]> (cherry picked from commit d018e6e) * updates with latest snowflake prometheus exporter (fixes null issues) (#939) * updates with latest snowflake prometheus exporter (fixes null issues) * Update CHANGELOG.md Co-authored-by: William Dumont <[email protected]> --------- Co-authored-by: William Dumont <[email protected]> (cherry picked from commit 551d407) * feat(vcs): bubble up SSH key conversion error for better debugging experience (#943) * feat(vcs): bubble up SSH key conversion error for better debugging experience Signed-off-by: hainenber <[email protected]> * chore: refactor code to be more succinct Signed-off-by: hainenber <[email protected]> --------- Signed-off-by: hainenber <[email protected]> (cherry picked from commit 037893f) * prepare changelog for 1.1.1 (#958) This includes all bugfixes found in main to date except for #703, which is a more involved change that should probably wait for a minor release. (cherry picked from commit 3bceb1a) --------- Co-authored-by: Weifeng Wang <[email protected]> Co-authored-by: mattdurham <[email protected]> Co-authored-by: William Dumont <[email protected]> Co-authored-by: Matthew Penner <[email protected]> Co-authored-by: Paschalis Tsilias <[email protected]> Co-authored-by: Stefan Kurek <[email protected]> Co-authored-by: Đỗ Trọng Hải <[email protected]>
PR Description
This PR decouples the Alloy component IDs from the generated types used for OTel Collector components internally; this fixes issues where long block labels could exceed the 63-character limit recently set by the upstream opentelemetry packages.
It is basically similar to what we've been doing with the jaegerremotesampling extension
alloy/internal/component/otelcol/extension/jaeger_remote_sampling/internal/jaegerremotesampling/factory.go
Lines 19 to 27 in 1cdd942
Which issue(s) this PR fixes
No issue filed
Notes to the Reviewer
I don't think this as a user-facing change so I haven't added a changelog entry.
PR Checklist