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

[exporters/datadog]: improve datadog_add_span_events #9997

Closed

Conversation

ehsannm
Copy link

@ehsannm ehsannm commented May 13, 2022

This is an Enhancement to the #2338.

Basically the current translation from open telemetry span events to span tags creates a big json string which really hard to read. Since datadog pretty the tags which are segmented with dots in the tag's name i suggested to have separate tags per event. Also for backward compatibility created a new config for it.

@ehsannm ehsannm requested a review from a team May 13, 2022 07:53
@ehsannm ehsannm requested a review from mx-psi as a code owner May 13, 2022 07:53
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 13, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: ehsannm / name: Ehsan Noureddin Moosa (7aac4a9)

@mx-psi
Copy link
Member

mx-psi commented May 13, 2022

@gbbr (or someone else from APM) to review

// single: converts all the events to a single json object
// multi: converts each span event to a separate tag.
// The default value is `single`
SpanEventsAsTags string `mapstructure:"span_events_as_tags"`
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using a string, let's create a new type alias like we do here:

// CumulativeMonotonicSumMode is the export mode for OTLP Sum metrics.
type CumulativeMonotonicSumMode string
const (
// CumulativeMonotonicSumModeToDelta calculates delta for
// cumulative monotonic sum metrics in the client side and reports
// them as Datadog counts.
CumulativeMonotonicSumModeToDelta CumulativeMonotonicSumMode = "to_delta"
// CumulativeMonotonicSumModeRawValue reports the raw value for
// cumulative monotonic sum metrics as a Datadog gauge.
CumulativeMonotonicSumModeRawValue CumulativeMonotonicSumMode = "raw_value"
)
var _ encoding.TextUnmarshaler = (*CumulativeMonotonicSumMode)(nil)
// UnmarshalText implements the encoding.TextUnmarshaler interface.
func (sm *CumulativeMonotonicSumMode) UnmarshalText(in []byte) error {
switch mode := CumulativeMonotonicSumMode(in); mode {
case CumulativeMonotonicSumModeToDelta,
CumulativeMonotonicSumModeRawValue:
*sm = mode
return nil
default:
return fmt.Errorf("invalid cumulative monotonic sum mode %q", mode)
}
}

Also, wondering if we should have this on a traces::span_events section, and call it... tags_mapping?

// get events as just a general tag
if s.Events().Len() > 0 {
tags[eventsTag] = eventsToString(s.Events())
if eventsLen := s.Events().Len(); eventsLen > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Note that we are in the process of moving part of the code to the Datadog Agent (see #9693, pending stability of the pdata module) so it may take a little until this is mergeable

@gbbr
Copy link
Member

gbbr commented May 13, 2022

I'll just leave a note here saying that we'll soon be looking into a better way to handle OpenTelemetry APM events at Datadog. Until then, I'd suggest not changing this to avoid breaking users even more

Lastly, indeed #9693 changes the code significantly to using what's in the Datadog Agent, but ultimately it should work out as the same behaviour. If we do decide to make this change, it should be made over there.

@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 May 28, 2022
@mx-psi mx-psi removed the Stale label May 30, 2022
@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 Jun 14, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jul 1, 2022

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants