feat(otel): allow to configure the faas.trigger attribute of the span #903
Conversation
| tracer_provider.force_flush(); | ||
| }) | ||
| // Set the "faas.trigger" attribute of the span (defaults to "http" otherwise) | ||
| .with_trigger("datasource"), |
There was a problem hiding this comment.
updated to use "pubsub" since "datasource" is the new default value
lambda-runtime/src/layers/otel.rs
Outdated
| otel_attribute_trigger: self | ||
| .otel_attribute_trigger | ||
| .clone() | ||
| .unwrap_or_else(|| "http".to_string()), |
There was a problem hiding this comment.
I have a doubt here, I don't know if the OpenTelemetryLayer::layer() is executed each time the lambda is invoked... or if it's call only on coldstarts during the Runtime construction
because if it's called for each lambda invocation, maybe we should do best than that to avoid performance issues?
For example, we could defaults to http directly in the OpenTelemetryLayer::new() function... but it would allocate a String for nothing when users will use with_trigger right after (i.e. OpenTelemetryLayer::new().with_trigger("datasource") would allocate http for nothing)
There was a problem hiding this comment.
layer() is only executed when the runtime starts, so this would be invoked only once. It makes me wonder if datasource should be the default, it's more generic than http. I'm ok with one allocation to initialize the service.
There was a problem hiding this comment.
looking at the spec, maybe using an enum for the datasource makes more sense since there are only a few trigger types defined.
There was a problem hiding this comment.
It makes me wonder if datasource should be the default, it's more generic than http
Yes datasource or even other, but this would be a breaking change, so let me know if you really want me to change the current default value 🙏
looking at the spec, maybe using an enum for the datasource makes more sense since there are only a few trigger types defined
The OpenTelemetry specs do not seem to move fast, but if they do, don't you think that this could be an issue for the users to wait for the aws-lambda-runtime releases to keep up with the OpenTelemetry specs evolution?
Also note that the current specs for faas.trigger are still experimental... and even the opentelemetry_semantic_conventions does not expose such an enum yet (don't know why though) 🤷
That being said, let me know if we should expose an enum here (should we name it OpenTelemetryFaasTrigger?) 🙏
OpenTelemetry docs for reference
There was a problem hiding this comment.
Yes
datasourceor evenother, but this would be a breaking change, so let me know if you really want me to change the current default value 🙏
This is fine. I don't think anyone is really using this yet.
That being said, let me know if we should expose an enum here (should we name it OpenTelemetryFaasTrigger?) 🙏
That name is fine by me. I understand that's experimental, but if someone wants to use it, it's hard to know what's supported on a string. People can still use other if at some point there are more types and they need to wait for a release.
Oliboy50
force-pushed
the
oliboy50-configure-faas-trigger
branch
from
74d5c51 to
5d824be
Compare
📬 Issue #, if available:
#898 (comment)
✍️ Description of changes:
Allow to configure the
faas.triggerattribute of the OpenTelemetry layer span which is currently hardcoded tohttpSmall breaking change: the
faas.triggerdefault value is nowdatasourceinstead ofhttp🔏 By submitting this pull request
cargo +nightly fmt.cargo clippy --fix.