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

Upgrade OTel to stop Tokio being pinned #2932

Merged
merged 3 commits into from
Nov 21, 2024
Merged

Conversation

itowlson
Copy link
Contributor

@calebschoepp I couldn't figure out how to migrate the custom error handler so I put in as much of their "migration guide" as I could get to compile, and left the commented-out custom handler for now. If you have a moment to take a look and figure out the correct migration, that would be awesome. Thank you so much for offering to pitch in.

The rest of it is my best effort at understanding how the APIs have changed from 0.25 to 0.27, but this really has been a matter of "follow the compiler errors" rather than any genuine understanding. And I have no real way of testing it. So please do read critically and err on the side of flagging anything that isn't obviously right! Thanks!

Copy link
Collaborator

@calebschoepp calebschoepp left a comment

Choose a reason for hiding this comment

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

I ran this locally and confirmed that traces, metrics, and logs are still correctly coming through 👍.

One oddity is that I'm observing some DEBUG logs that I feel like I shouldn't be seeing:

2024-11-21T16:39:35.806231Z DEBUG opentelemetry_sdk:  name="PeriodicReader.ExportTriggered" Export message received.
2024-11-21T16:39:35.821313Z DEBUG opentelemetry_sdk:  name="PeriodicReader.ExportTriggered" Export message received.
2024-11-21T16:40:35.805557Z DEBUG opentelemetry_sdk:  name="PeriodicReader.ExportTriggered" Export message received.

Investigating ^

let exporter_builder: LogExporterBuilder = match OtlpProtocol::logs_protocol_from_env() {
OtlpProtocol::Grpc => opentelemetry_otlp::new_exporter().tonic().into(),
OtlpProtocol::HttpProtobuf => opentelemetry_otlp::new_exporter().http().into(),
let exporter = match OtlpProtocol::logs_protocol_from_env() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

All these log changes seem correct to me.

let exporter_builder: MetricsExporterBuilder = match OtlpProtocol::metrics_protocol_from_env() {
OtlpProtocol::Grpc => opentelemetry_otlp::new_exporter().tonic().into(),
OtlpProtocol::HttpProtobuf => opentelemetry_otlp::new_exporter().http().into(),
let exporter = match OtlpProtocol::metrics_protocol_from_env() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Metrics changes also make sense to me.

let exporter_builder: SpanExporterBuilder = match OtlpProtocol::traces_protocol_from_env() {
OtlpProtocol::Grpc => opentelemetry_otlp::new_exporter().tonic().into(),
OtlpProtocol::HttpProtobuf => opentelemetry_otlp::new_exporter().http().into(),
let exporter = match OtlpProtocol::traces_protocol_from_env() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You also get a ✅ here to 😄

tracing::trace!(?err, "OpenTelemetry error");
}
}
// fn otel_error_handler(err: opentelemetry::global::Error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll try to get this working

@calebschoepp
Copy link
Collaborator

Dumping my stream of consciousness here as I work on this.

The errors I'm seeing are b/c of you following the error handler migration guide.

@calebschoepp
Copy link
Collaborator

This should do it @itowlson itowlson#1. Once you have that merged I think this all LGTM.

Fix OTel error handler migration
@itowlson itowlson marked this pull request as ready for review November 21, 2024 18:48
@itowlson itowlson enabled auto-merge November 21, 2024 20:54
@itowlson itowlson merged commit 8d27d0d into fermyon:main Nov 21, 2024
17 checks passed
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