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

[bugfix/tracing] fix broken tracing due to conflicting schema url #2712

Merged
merged 1 commit into from
Mar 4, 2024

Conversation

milas
Copy link
Contributor

@milas milas commented Mar 3, 2024

Description

The OpenTelemetry SDK is very strict about the schema version when the Resource is initialized.

Specifically, different schema versions CANNOT be mixed, and since the default SDK resource (which is merged with the user-defined one) defines a schema URL, the semconv imports are really prone to being out-of-sync.

The best way to avoid this is to merge a schemaless resource. This is fine...there's plenty of other ways to get semconv out of sync, and the core service attributes (e.g. service.name) should not ever change.

Additionally, any errors here are now propagated so that they'll be visible instead of silently swallowed.

Checklist

  • I/we have read the GoToSocial contribution guidelines.
  • I/we have discussed the proposed changes already, either in an issue on the repository, or in the Matrix chat.
  • I/we have not leveraged AI to create the proposed changes.
  • I/we have performed a self-review of added code.
  • I/we have written code that is legible and maintainable by others.
  • I/we have commented the added code, particularly in hard-to-understand areas.
  • I/we have made any necessary changes to documentation.
  • I/we have added tests that cover new code.
  • I/we have run tests and they pass locally with the changes.
  • I/we have run go fmt ./... and golangci-lint run.

The OpenTelemetry SDK is very strict about the schema version when
the `Resource` is initialized.

Specifically, different schema versions _CANNOT_ be mixed, and since
the default SDK resource (which is merged with the user-defined one)
defines a schema URL, the `semconv` imports are really prone to being
out-of-sync.

The best way to avoid this is to merge a _schemaless_ resource. This
is fine...there's plenty of other ways to get `semconv` out of sync,
and the core service attributes (e.g. `service.name`) should not ever
change.

Additionally, any errors here are now propagated so that they'll be
visible instead of silently swallowed.
@tsmethurst tsmethurst changed the title fix(tracing): fix broken tracing due to conflicting schema url [bugfix/tracing] fix broken tracing due to conflicting schema url Mar 3, 2024
@tsmethurst
Copy link
Contributor

tsmethurst commented Mar 3, 2024

Ah thanks for this :)

So this won't cause a regression to the issue described here #2503 ?

@daenney
Copy link
Member

daenney commented Mar 4, 2024

Ah yeah, that seems better. Then we should be able to update the otel dependencies too going forwards without problems.

@tsmethurst tsmethurst merged commit 66d9297 into superseriousbusiness:main Mar 4, 2024
2 checks passed
@milas milas deleted the fix-otel-tracing branch March 5, 2024 01:38
@milas
Copy link
Contributor Author

milas commented Mar 5, 2024

So this won't cause a regression to the issue described here #2503 ?

Nope! Using schemaless should insulate us from it breaking again.

Longer explanation follows with more than you want to know because the semconv situation is...weird, and I've broken & fixed it across several Docker1 repos now 🙈


semconv are well-known/standardized fields, and while there have been breaking changes, the service.name key is pretty fundamental and changing it would break EVERYTHING OTel-related, so I don't think it's a risk.

Another possibility is to add a test for constructing the Resource, which would catch the breakage when updating go.mod in CI, but that's a bit of a nuisance since it means looking at the OTel SDK source to find what semconv version it uses.

Footnotes

  1. disclaimer: I work at Docker Inc but my contributions to GoToSocial are my own work and not sponsored or endorsed by them.

daenney added a commit that referenced this pull request Mar 6, 2024
Same deal as #2712, just on the metrics.
@daenney daenney mentioned this pull request Mar 6, 2024
10 tasks
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.

3 participants