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

otelcol: decouple otel/alloy component IDs #882

Merged
merged 4 commits into from
May 23, 2024

Conversation

tpaschalis
Copy link
Member

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

const (
// The value of extension "type" in configuration.
typeStr = "jaegerremotesampling"
)
// NewFactory creates a factory for the jaeger remote sampling extension.
func NewFactory() extension.Factory {
return extension.NewFactory(
otelcomponent.MustNewType(typeStr),

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

  • CHANGELOG.md updated (N/A)
  • Documentation added (N/A)
  • Tests updated
  • Config converters updated (N/A)

Signed-off-by: Paschalis Tsilias <paschalist0@gmail.com>
@tpaschalis tpaschalis requested review from ptodev and rfratto May 16, 2024 16:52
// Inform listeners that our handler changed.
a.opts.OnStateChange(Exports{
Handler: Handler{
ID: otelcomponent.NewID(otelcomponent.MustNewType(cTypeStr)),
ID: otelcomponent.NewID(otelcomponent.MustNewType(typeStr)),
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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 <paschalist0@gmail.com>
@tpaschalis tpaschalis requested a review from rfratto May 20, 2024 10:15
@rfratto
Copy link
Member

rfratto commented May 22, 2024

@tpaschalis

Based on this:

this fixes issues where long block labels could exceed the 63-character limit recently set by the upstream opentelemetry packages.

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).

tpaschalis and others added 2 commits May 23, 2024 12:22
Signed-off-by: Paschalis Tsilias <paschalist0@gmail.com>
@tpaschalis tpaschalis enabled auto-merge (squash) May 23, 2024 09:51
@tpaschalis tpaschalis merged commit d018e6e into main May 23, 2024
18 checks passed
@tpaschalis tpaschalis deleted the decouple-component-id-otelcol-id branch May 23, 2024 09:55
rfratto pushed a commit to rfratto/alloy that referenced this pull request May 30, 2024
Signed-off-by: Paschalis Tsilias <paschalist0@gmail.com>
(cherry picked from commit d018e6e)
rfratto added a commit that referenced this pull request May 30, 2024
* Fix panic when component ID contains `/` in `otelcomponent.MustNewType(ID)` (#858)

Signed-off-by: Weifeng Wang <qclaogui@gmail.com>
(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 <39704712+erikbaranowski@users.noreply.github.com>

---------

Co-authored-by: Erik Baranowski <39704712+erikbaranowski@users.noreply.github.com>
(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 <me@matthewp.io>
(cherry picked from commit 3df2cd0)

* otelcol: decouple otel/alloy component IDs (#882)

Signed-off-by: Paschalis Tsilias <paschalist0@gmail.com>
(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 <william.dumont@grafana.com>

---------

Co-authored-by: William Dumont <william.dumont@grafana.com>
(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 <dotronghai96@gmail.com>

* chore: refactor code to be more succinct

Signed-off-by: hainenber <dotronghai96@gmail.com>

---------

Signed-off-by: hainenber <dotronghai96@gmail.com>
(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 <qclaogui@gmail.com>
Co-authored-by: mattdurham <mattdurham@ppog.org>
Co-authored-by: William Dumont <william.dumont@grafana.com>
Co-authored-by: Matthew Penner <me@matthewp.io>
Co-authored-by: Paschalis Tsilias <tpaschalis@users.noreply.github.com>
Co-authored-by: Stefan Kurek <stefan.kurek@observiq.com>
Co-authored-by: Đỗ Trọng Hải <41283691+hainenber@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants