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

Feature/otel span kind support #946

Merged
merged 5 commits into from
Dec 12, 2024

Conversation

alessandrobologna
Copy link
Contributor

📬 *Issue #945

✍️ *Add OpenTelemetry span kind support to Lambda invocation tracing. This enhancement allows users to specify the span kind (e.g., SERVER, CLIENT) in their Lambda handlers, improving how functions are represented in distributed traces.
In this way:

  • Users can set the span kind in their handlers using span.record("otel.kind", "SERVER") or `span.record("otel.kind", "CONSUMER") etc.
  • Allows to follow OpenTelemetry semantic conventions for span kind while maintaining backward compatibility (the field can be left empty)

Changes:

  • Added otel.kind field to the Lambda invocation span, initialized as field::Empty as per the tracing_opentelemetry docs
  • Updated the example in examples/opentelemetry-tracing/main.rsto demonstrate setting the span kind

Please note that since tracing_opentelemetry 0.28.0, they have provided a method to set the status on a span, therefore setting otel.status_code and otel.status_message in advance on the span is not required anymore.

🔏 By submitting this pull request

  • I confirm that I've ran cargo +nightly fmt.
  • I confirm that I've ran cargo clippy --fix.
  • I confirm that I've made a best effort attempt to update all relevant documentation.
  • I confirm that my contribution is made under the terms of the Apache 2.0 license.

@maxday
Copy link
Contributor

maxday commented Dec 2, 2024

Thanks for your contribution!
What do you think about using the enum from https://docs.rs/opentelemetry/0.27.0/opentelemetry/trace/enum.SpanKind.html instead of passing an hardcoded string like server?

@alessandrobologna
Copy link
Contributor Author

@maxday that's really depending on how the tracing_opentelemetry crate handles it. As it is today, it handles string to SpanKind conversion internally: see https://github.com/tokio-rs/tracing-opentelemetry/blob/v0.1.x/src/layer.rs#L443 and https://github.com/tokio-rs/tracing-opentelemetry/blob/v0.1.x/src/layer.rs#L102-L111.

@maxday
Copy link
Contributor

maxday commented Dec 4, 2024

While testing it seems that the "kind" is passed as a integer (enum id I guess)
for instance 4 forproducer "parentSpanId":"0000000000000000","name":"helloooo","kind":4,"startTimeUnixNano":1733334175621925826
It seems that's not what we really want, I think it would be better to see the full name of the kind? (see: https://opentelemetry.io/docs/specs/otel/trace/api/#spankind)

Also not providing any otel.kind in a span results in having kind:1 in the trace which is also not a wanted side effect?WDYT?

@alessandrobologna
Copy link
Contributor Author

Yes, it's a little bit confusing, but the serialization of SpanKind (an enum) is an int for the specs.
And the 1 value is actually meant to imply "internal", which is the default behavior when it's not specified (see the comment onSPAN_KIND_INTERNAL). So the rust sdk is actually following the standard, by representing it as a number.

@alessandrobologna
Copy link
Contributor Author

(oh, and the o11y platforms will typically, or at least all those that i tried, convert it back to a string for visualization)

@maxday
Copy link
Contributor

maxday commented Dec 5, 2024

It seems that our CI is red, I've just pushed a PR to fix it, I'll let you know once it gets reviewed/merged so you can rebase from that. Thanks!

@alessandrobologna
Copy link
Contributor Author

Yes, I saw, no worries

@maxday
Copy link
Contributor

maxday commented Dec 9, 2024

#948 has just been merged and CI is now green, could you please merge main into your branch, this should resolve the failing checks? Thanks!

…ind-support

Merge upstream main to resolve failing checks
@alessandrobologna
Copy link
Contributor Author

Ok, thank you. I think i have done it.

@maxday maxday self-requested a review December 9, 2024 16:29
Copy link
Contributor

@maxday maxday left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks 👍

@maxday maxday merged commit 9416d72 into awslabs:main Dec 12, 2024
7 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.

2 participants