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

[exporter/datadogexporter] Add logs support #13987

Merged

Conversation

dineshg13
Copy link
Member

@dineshg13 dineshg13 commented Sep 8, 2022

Description: Add logs support for datadog exporter
Link to tracking Issue: [issue] (#2651)

Testing:

Documentation: Will add examples in the follow up PR

@dineshg13 dineshg13 changed the title fix gosum [exporter/datadogexporter] Add logs support Sep 8, 2022
@dineshg13 dineshg13 marked this pull request as ready for review September 8, 2022 18:53
@dineshg13 dineshg13 requested a review from a team September 8, 2022 18:53
@dineshg13 dineshg13 requested a review from mx-psi as a code owner September 8, 2022 18:53
mx-psi
mx-psi previously requested changes Sep 9, 2022
exporter/datadogexporter/README.md Outdated Show resolved Hide resolved
exporter/datadogexporter/factory.go Outdated Show resolved Hide resolved
exporter/datadogexporter/factory.go Outdated Show resolved Hide resolved
exporter/datadogexporter/go.mod Outdated Show resolved Hide resolved
exporter/datadogexporter/internal/logs/sender.go Outdated Show resolved Hide resolved
exporter/datadogexporter/internal/testutils/test_utils.go Outdated Show resolved Hide resolved
exporter/datadogexporter/logs_exporter_test.go Outdated Show resolved Hide resolved
exporter/datadogexporter/internal/logs/translator.go Outdated Show resolved Hide resolved
exporter/datadogexporter/internal/logs/translator.go Outdated Show resolved Hide resolved
@dineshg13 dineshg13 force-pushed the dinesh.gurumurthy/add-logs-support-datadog branch from c2777d7 to 06721ee Compare September 10, 2022 16:51
@dineshg13 dineshg13 force-pushed the dinesh.gurumurthy/add-logs-support-datadog branch from 06721ee to 188386d Compare September 12, 2022 01:38
l.AdditionalProperties[otelSeverityNumber] = fmt.Sprintf("%d", lr.SeverityNumber())
} else if lr.SeverityText() != "" {
status = lr.SeverityText()
l.AdditionalProperties[otelSeverityText] = lr.SeverityText()
Copy link

Choose a reason for hiding this comment

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

Right now, if there is a SeverityNumber present, we skip this code block and we don't add the SeverityText as an additional property. I think the additional properties should be added regardless; that is if the SeverityText and/or SeverityNumber are present, we should always preserve them as otelSeverityText and/or otelSeverityNumber. The if/else logic should only apply to what we use for the official status.

Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

LGTM, no further comments from my side :)

Once Nick's comments are addressed this will be good to merge

Copy link

@nhinsch nhinsch left a comment

Choose a reason for hiding this comment

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

Approved pending a few minor last comments.

@dineshg13 dineshg13 requested a review from gbbr September 25, 2022 20:36
Copy link
Member

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

I have to review further, submitting what I have so far in case it helps.

exporter/datadogexporter/config.go Outdated Show resolved Hide resolved
exporter/datadogexporter/factory.go Outdated Show resolved Hide resolved
exporter/datadogexporter/factory.go Outdated Show resolved Hide resolved
exporter/datadogexporter/factory.go Show resolved Hide resolved
exporter/datadogexporter/factory_test.go Outdated Show resolved Hide resolved
exporter/datadogexporter/internal/logs/sender.go Outdated Show resolved Hide resolved
exporter/datadogexporter/internal/logs/sender.go Outdated Show resolved Hide resolved
exporter/datadogexporter/internal/logs/sender.go Outdated Show resolved Hide resolved
exporter/datadogexporter/internal/logs/translator.go Outdated Show resolved Hide resolved
exporter/datadogexporter/factory.go Show resolved Hide resolved
exporter/datadogexporter/factory_test.go Outdated Show resolved Hide resolved
exporter/datadogexporter/logs_exporter.go Show resolved Hide resolved
exporter/datadogexporter/logs_exporter.go Outdated Show resolved Hide resolved
exporter/datadogexporter/factory.go Show resolved Hide resolved
l.Service = datadog.PtrString(serviceName)
}

// we need to set log attributes as AdditionalProperties
Copy link
Member

Choose a reason for hiding this comment

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

Why? Can you explain in the comment please?

exporter/datadogexporter/internal/logs/translator.go Outdated Show resolved Hide resolved
exporter/datadogexporter/internal/logs/translator.go Outdated Show resolved Hide resolved
exporter/datadogexporter/factory.go Show resolved Hide resolved
exporter/datadogexporter/factory.go Outdated Show resolved Hide resolved
exporter/datadogexporter/factory_test.go Outdated Show resolved Hide resolved
exporter/datadogexporter/internal/logs/sender.go Outdated Show resolved Hide resolved
exporter/datadogexporter/internal/testutils/test_utils.go Outdated Show resolved Hide resolved
exporter/datadogexporter/internal/testutils/test_utils.go Outdated Show resolved Hide resolved
exporter/datadogexporter/logs_exporter_test.go Outdated Show resolved Hide resolved
exporter/datadogexporter/logs_exporter_test.go Outdated Show resolved Hide resolved
Comment on lines +126 to +127
// traceIDToUint64 converts 128bit traceId to 64 bit uint64
func traceIDToUint64(b [16]byte) uint64 {
Copy link
Member

Choose a reason for hiding this comment

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

These should be shared but that's for another PR.

@dineshg13 dineshg13 force-pushed the dinesh.gurumurthy/add-logs-support-datadog branch from 1574385 to f246e2f Compare September 27, 2022 00:01
Copy link
Member

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

Please go through all the comments in the PR and make sure they are resolved. You've omitted some of them.

exporter/datadogexporter/config.go Outdated Show resolved Hide resolved
exporter/datadogexporter/factory.go Show resolved Hide resolved
@@ -0,0 +1,18 @@
// Copyright The OpenTelemetry Authors
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Copyright The OpenTelemetry Authors
// Copyright The OpenTelemetry Authors

@@ -0,0 +1,72 @@
// Copyright The OpenTelemetry Authors
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Copyright The OpenTelemetry Authors
// Copyright The OpenTelemetry Authors

// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// http://www.apache.org/licenses/LICENSE-2.0
// http://www.apache.org/licenses/LICENSE-2.0

exporter/datadogexporter/internal/logs/translator.go Outdated Show resolved Hide resolved
exporter/datadogexporter/logs_exporter.go Outdated Show resolved Hide resolved
exporter/datadogexporter/logs_exporter.go Outdated Show resolved Hide resolved
exporter/datadogexporter/logs_exporter.go Outdated Show resolved Hide resolved
exporter/datadogexporter/config.go Outdated Show resolved Hide resolved
@mx-psi mx-psi added the ready to merge Code review completed; ready to merge by maintainers label Sep 27, 2022
@mx-psi mx-psi merged commit 009ad32 into open-telemetry:main Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants