feat: Generate more telemetry via TelemetryAPI receiver #1230
Conversation
|
@rapphil I'm curious what you think about this. |
| "os" | ||
| "os/signal" | ||
| "path/filepath" | ||
| "sync" | ||
| "syscall" | ||
|
|
||
| "github.com/open-telemetry/opentelemetry-lambda/collector/lambdalifecycle" |
There was a problem hiding this comment.
How does this new component propagate context from the collector to the application?
There was a problem hiding this comment.
I'm not sure whether this answers your question but the collector doesn't really pass any context to the application. When the application generates a span A with a parent B that was propagated from an external system, the faas processor identifies span A (by checking for spans with a faas.invocation_id) and inserts the Lambda runtime span (let's call it C) between A and B. That means, C becomes parent of A and B becomes parent of C.
There was a problem hiding this comment.
This looks promising.
Aside from the comment I left on the ConsumeTraces method, I would be able to review this better in at least a couple of PRs. Supporting changes in the telemetryapireceiver would be the most beneficial.
Thanks for making this contribution. Looking forward to updates.
| return consumer.Capabilities{MutatesData: true} | ||
| } | ||
|
|
||
| func (p *faasProcessor) ConsumeTraces(ctx context.Context, td ptrace.Traces) error { |
There was a problem hiding this comment.
Simplifying this method will help with reasoning about this code and testing paths. For example, these high-level operations make sense to me:
- categorizeSpan (caches with runtimeSpans, initSpans, or invocationSpans)
- associateSpans (adds traceid and parentid)
- forwardRemainingSpans
There was a problem hiding this comment.
This looks promising.
Aside from the comment I left on the ConsumeTraces method, I would be able to review this better in at least a couple of PRs. Supporting changes in the telemetryapireceiver would be the most beneficial.
Thanks for making this contribution. Looking forward to updates.
Adding notes below: 2024-05-23
After some discussion, I think this would be ideal to merge as something entirely elective. I haven't worked out how to refactor this to a processor, but that would be the best path to make this available. A fallback would be to keep it a receiver as an alternative to coldstart rather than replacement.
Also note that the spirit of this review was a high-level opinion on whether the code belonged in the project and a couple of changes to facilitate a more in depth review.
|
@borchero Do you have interest in finishing this out or should we close the PR? I think it's a promising improvement. |
Motivation
The current implementation of the Telemetry API receiver only generates init spans. However, it can easily be extended to create
This PR leverages the events provided by the Telemetry API to implement the latest semantic conventions for FaaS spans and metrics.
It also updates the
coldstartprocessorto become a more generalfaasprocessor.Please read through the READMEs of the
telemetryapnireceiverand thefaasprocessorfor an overview of the changes.I did not yet write unit tests (and only checked functionality manually) but wanted to check with the maintainers first whether this change would be desirable to include into this project.
This PR supersedes #1216.