[exporter/azuremonitorexporter] fix: correct parent for span events #30659
Conversation
| @@ -133,6 +133,7 @@ func spanToEnvelopes( | |||
| } | |||
|
|
|||
| spanEventEnvelope := newEnvelope(span, toTime(spanEvent.Timestamp()).Format(time.RFC3339Nano)) | |||
| spanEventEnvelope.Tags[contracts.OperationParentId] = traceutil.SpanIDToHexOrEmptyString(span.SpanID()) | |||
There was a problem hiding this comment.
newEnvelope method has the entry for envelope.Tags[contracts.OperationParentId], that seems to be correct.
envelope.Tags[contracts.OperationParentId] = traceutil.SpanIDToHexOrEmptyString(span.ParentSpanID())
spanEventEnvelope.Tags[contracts.OperationParentId] = traceutil.SpanIDToHexOrEmptyString(span.SpanID())
Setting the current span ID as the parent does not seem correct. Did you validate with an app that issue #27233 is resolved by this update?
Providing the .NET implementation here for reference - https://github.com/Azure/azure-sdk-for-net/blob/2d1996d14d430c541c5beabad612b98a01fb4a4e/sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Customizations/Models/TelemetryItem.cs#L19
There was a problem hiding this comment.
To be honest I haven't validated the change in conjunction with AppInsights.
But I've made up for it now and the result looks correct:
Before the change:
Now:
There was a problem hiding this comment.
@rajkumar-rangaraj - the nifty difference here - we are not mapping spans, we are mapping logs - e.g. compare with https://github.com/Azure/azure-sdk-for-java/blob/9e6bab9bb50d865ed8b4e5ab3198e3fc56d9e468/sdk/monitor/azure-monitor-opentelemetry-exporter/src/main/java/com/azure/monitor/opentelemetry/exporter/implementation/LogDataMapper.java#L186 and lines around.
There was a problem hiding this comment.
Yes, this is correct in the context of logs. All logs will have the parent ID labeled as 'spanid.' However, you are modifying the trace envelope, which corresponds to spans. Logs are handled differently; therefore, the change should be applied to the log exporter file. The current proposed change might break the UI in other scenarios.
There was a problem hiding this comment.
Moving your changes to log_to_envelope.go should resolve an issue.
There was a problem hiding this comment.
@rajkumar-rangaraj - first huge sorry - with the LogDataMapperI've pointed you to the wrong direction. We are still and only talking about trace data. However I've found the more correct line that underpins my change also in the azure-sdk-for-java - https://github.com/Azure/azure-sdk-for-java/blob/9e6bab9bb50d865ed8b4e5ab3198e3fc56d9e468/sdk/monitor/azure-monitor-opentelemetry-exporter/src/main/java/com/azure/monitor/opentelemetry/exporter/implementation/SpanDataMapper.java#L779
For Events the parent span is the span of the current operation. That also makes sense, because the events are just annotations of the current operation/tracing span. So maybe we should take one step back, and find out how far we already agree.
- There is a problem with the original implementation (azuremonitorexporter assign wrong parentId to span events #27233) - the original behaviour could and should be improved?
- The provided PR solves this issue - and has no unintended side effects?
- Is the form of the changes okay, or should the patch be rephrased?
There was a problem hiding this comment.
Got it. It makes sense for span events.
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
@pcwiese, Could you please review this PR. |
@alaendle Could you please fix this one? |
|
@open-telemetry/collector-contrib-approvers Could you please approve the pipeline and help merge this PR. It has got an approval from the code owner. |
alaendle
force-pushed
the
exporter-azuremonitorexporter/fix-parent-of-span-events
branch
from
62185d2
to
d4db28c
Compare
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Pinging @djaglowski - maybe you could approve/merge this PR? Or is still something missing? |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This is becoming more and more of a frustrating experience. The PR is now 3 month old and I still can't predict whether or when he will make it to the finish line. |
bryan-aguilar
added
ready to merge
|
Apologies @alaendle. I understand how this can be frustrating. I see we have codeowner and approver review already. I went ahead and stuck the |
Description: Corrected parent for exported span events.
For details please take a look at the corresponding issue.
I think the change is pretty straight forward - one line added.
However I'm not a go-expert - so there might exist a more elegant implementation.
Link to tracking Issue: #27233
Testing: Adapted existing units test.
Documentation: n/a
(fyi @pcwiese)