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

[receiver/datadog] add datadog span and trace id #23058

Merged
merged 1 commit into from
Aug 4, 2023

Conversation

povilasv
Copy link
Contributor

@povilasv povilasv commented Jun 5, 2023

Description:

Add datadog trace id and span id so we cancorrelate traces received by datadogreceiver with logs.

Datadog libraries automatically provide datadog span ids and trace ids with the logs for various languages:

Example log for python:

2023-06-05 06:01:30,459 INFO [__main__] [test-enhanced.py:44] [dd.service= dd.env= dd.version= dd.trace_id=6249785623524942554 dd.span_id=2
28114450199004348] - Starting a new Game!

Link to tracking Issue: #23057
Testing:

  • local testing with python app
  • updated unit tests

Documentation:

@@ -88,7 +89,8 @@ func toTraces(payload *pb.TracerPayload, req *http.Request) ptrace.Traces {
if span.Error > 0 {
newSpan.Status().SetCode(ptrace.StatusCodeError)
}

newSpan.Attributes().PutStr("datadog.span.id", strconv.FormatUint(span.SpanID, 10))
Copy link
Member

@boostchicken boostchicken Jun 11, 2023

Choose a reason for hiding this comment

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

I would be nice to have these strings in an enum like semconv. Better yet, making a proposal to get them in semconv. The actual spec shouldn't have DD specific stuff, but I don't think it would be a problem to add in the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MovieStoreGuy
Copy link
Contributor

Can I ask what are you trying to visualise this data in? Is there any reason why you can't use the trace ID and span ID that is already sent?

I don't believe adding span field to an attribute is a good idea, it is wasteful since it is duplicating data.

@povilasv
Copy link
Contributor Author

povilasv commented Jun 13, 2023

Can I ask what are you trying to visualise this data in? Is there any reason why you can't use the trace ID and span ID that is already sent?

I don't believe adding span field to an attribute is a good idea, it is wasteful since it is duplicating data.

@MovieStoreGuy thanks for the review!

I sort of agree, but I see this is a temporary way of correlating logs and traces:

Since datadog library give log in this format:

2023-06-05 06:01:30,459 INFO [main] [test-enhanced.py:44] [dd.service= dd.env= dd.version= dd.trace_id=6249785623524942554 dd.span_id=2
28114450199004348] - Starting a new Game!

And atm I'm not sure how to parse this trace id and span id to use for correlation in opentelemetry?

I know otlp logs have dedicated field for SpanId and TraceID. Should we instead use some processor to transform logs in this format?

But I'm not sure where the best place to contribute parsing from datadog trace id to otel trace id would be?

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@jpkrohling
Copy link
Member

Since datadog library give log in this format:

Aren't those trace IDs and span IDs the same as already set a few lines above?

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Jul 25, 2023
@jpkrohling
Copy link
Member

ping @povilasv

@github-actions github-actions bot removed the Stale label Jul 27, 2023
@povilasv
Copy link
Contributor Author

Hey @jpkrohling .Ah sorry forgot to reply, so coming back to the problem. Datadog instrumentation gives trace ids in this format:

2023-06-05 06:01:30,459 INFO [__main__] [test-enhanced.py:44] [dd.service= dd.env= dd.version= dd.trace_id=6249785623524942554 dd.span_id=2
28114450199004348] - Starting a new Game!

This receiver encodes DD TraceID's using:

newSpan.SetTraceID(uInt64ToTraceID(0, span.TraceID))


func uInt64ToTraceID(high, low uint64) pcommon.TraceID {
	traceID := [16]byte{}
	binary.BigEndian.PutUint64(traceID[:8], high)
	binary.BigEndian.PutUint64(traceID[8:], low)
	return traceID
}

Basically binary encoding.

So question - how do we correlate the binray encoded trace ID with raw DD trace id from the log?

I don't think in log storage systems it's common to have such decoding functions. But I might be wrong here.

So it would be ideal we would have same exact representation, so we can easily switch from log's trace id to span.

@jpkrohling
Copy link
Member

Understood, and it makes sense to me then!

@jpkrohling
Copy link
Member

@boostchicken , @MovieStoreGuy , can you take another look at this one?

@boostchicken
Copy link
Member

@boostchicken , @MovieStoreGuy , can you take another look at this one?

Sure thing

@povilasv povilasv force-pushed the dd-receiver-span-id branch 2 times, most recently from 8b31e91 to 1260880 Compare July 28, 2023 09:45
Copy link
Contributor

@MovieStoreGuy MovieStoreGuy left a comment

Choose a reason for hiding this comment

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

I understand why this is needed but ideally I would like to see this work be done by a connector in a more generic sense

@povilasv
Copy link
Contributor Author

povilasv commented Aug 3, 2023

Hey @boostchicken, would appreciate another look :)

@jpkrohling
Copy link
Member

I'm merging this as it's only about adding two new attributes. If @boostchicken has concerns, we can revert or work on this on a follow-up PR.

@jpkrohling jpkrohling merged commit 29f6acf into open-telemetry:main Aug 4, 2023
90 checks passed
@github-actions github-actions bot added this to the next release milestone Aug 4, 2023
@boostchicken
Copy link
Member

No major concerns fine as is, my feedback was more a matter of opinion than best practice :)

@clintonb
Copy link

clintonb commented Feb 1, 2024

I recognize I'm late to the party, but what was the goal of this change? It makes sense to add the dd.trace_id and dd.span_id attributes to logs. What's the point of adding them to traces and spans? What is using these datadog.* attributes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants