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

Collect user provided tracer name, version and schemaURL #844

Merged

Conversation

RonFed
Copy link
Contributor

@RonFed RonFed commented May 17, 2024

When the WithGlobal InstrumentationOption is used, use the user provided tracer name.

@RonFed RonFed requested a review from a team May 17, 2024 20:03
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Tracer name, version, and scope are the three identifying fields. By only using the tracer name here we are going to have collisions with the wrong tracer being used. The version and schema URL need to also be determined and used in the tracer resolution path.

internal/pkg/opentelemetry/controller.go Outdated Show resolved Hide resolved
internal/pkg/opentelemetry/controller.go Show resolved Hide resolved
@RonFed
Copy link
Contributor Author

RonFed commented May 31, 2024

The Tracer name, version, and scope are the three identifying fields. By only using the tracer name here we are going to have collisions with the wrong tracer being used. The version and schema URL need to also be determined and used in the tracer resolution path.

I added the version. Regarding the schema, I noticed it was added recently (open-telemetry/opentelemetry-go#5426) to the traces map that the probe is reading, so currently it is not being read by the probe. We can add it once a new release of otel-go is out (and then make the adjustment in the probe to only look for the schema in version newer from that release).

@RonFed RonFed requested a review from MrAlias May 31, 2024 21:29
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

Copy link
Contributor

@grcevski grcevski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@RonFed
Copy link
Contributor Author

RonFed commented Jun 13, 2024

@MrAlias I think this will break once the new version of otel-go is released (since the tracers' map key size will change), so maybe it is better to wait with this PR. Once the new version is out I will add code to adjust the key size

@MrAlias
Copy link
Contributor

MrAlias commented Jul 2, 2024

Working on otel release here: open-telemetry/opentelemetry-go#5569

@MrAlias
Copy link
Contributor

MrAlias commented Jul 3, 2024

@RonFed Releases v1.28.0/v0.50.0/v0.4.0 was made yesterday. This should be unblocked.

@RonFed RonFed changed the title Collect user provided tracer name and use it for scope name Collect user provided tracer name, version and schemaURL Jul 5, 2024
@MrAlias MrAlias added this to the v0.14.0-alpha milestone Jul 9, 2024
@RonFed RonFed force-pushed the otelglobal_collect_user_tracer_name branch from 773a781 to 4c0b974 Compare July 10, 2024 07:54
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me overall. Minor nits.

internal/pkg/opentelemetry/controller.go Show resolved Hide resolved
internal/test/e2e/otelglobal/main.go Outdated Show resolved Hide resolved
internal/test/e2e/otelglobal/traces.json Outdated Show resolved Hide resolved
@MrAlias
Copy link
Contributor

MrAlias commented Jul 12, 2024

Looks like some tests need to be updated for the added schema URLs.

@MrAlias MrAlias merged commit c244026 into open-telemetry:main Jul 12, 2024
18 checks passed
@MrAlias MrAlias mentioned this pull request Jul 15, 2024
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