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/datadog] Update metrics export to use datadog-api-client-go instead of Zorkian #16545

Merged
merged 6 commits into from
Jan 4, 2023

Conversation

songy23
Copy link
Member

@songy23 songy23 commented Nov 30, 2022

Description:

Migrate metrics export in datadogexporter from gopkg.in/zorkian/go-datadog-api.v2 to github.com/DataDog/datadog-api-client-go/v2/api/datadog because the Zorkian library is deprecated. This is guarded by feature gate and can be disabled by adding CLI flag --feature-gates=-exporter.datadogexporter.metricexportnativeclient

Link to tracking Issue:

Fixes #16776.
Fixes #6944.

Testing:

Tested in the staging environment, verified OTLP metrics are properly displayed on Datadog UI.

Documentation:

@github-actions github-actions bot added the exporter/datadog Datadog components label Nov 30, 2022
@songy23 songy23 force-pushed the ddog-metric-export branch 3 times, most recently from c5acbf0 to ac9807f Compare December 2, 2022 20:32
@runforesight
Copy link

runforesight bot commented Dec 2, 2022

Foresight Summary

    
Major Impacts
Foresight hasn't detected any major impact on your workflows and tests.

View More Details

 build-and-test workflow has finished in 20 minutes 48 seconds (39 minutes 27 seconds less than main branch avg.) and finished at 3rd Jan, 2023.


Job Failed Steps Tests
integration-tests N/A  N/A See Details

🔎 See details on Foresight

*You can configure Foresight comments in your organization settings page.

@songy23 songy23 force-pushed the ddog-metric-export branch 7 times, most recently from 1efdeb6 to ea5539a Compare December 8, 2022 13:58
@songy23 songy23 force-pushed the ddog-metric-export branch 2 times, most recently from 0d4c058 to 054548f Compare December 16, 2022 16:50
@songy23 songy23 changed the title [WIP] [exporter/datadog] Update metrics export to use datadog-api-client-go instead of Zorkian [exporter/datadog] Update metrics export to use datadog-api-client-go instead of Zorkian Dec 16, 2022
@songy23 songy23 marked this pull request as ready for review December 16, 2022 16:52
@songy23 songy23 requested a review from a team December 16, 2022 16:52
@songy23 songy23 requested a review from mx-psi as a code owner December 16, 2022 16:52
exporter/datadogexporter/factory_test.go Outdated Show resolved Hide resolved
configuration.UserAgent = UserAgent(buildInfo)
configuration.HTTPClient = NewHTTPClient(settings, insecureSkipVerify)
configuration.Compress = true
if endpoint != "" {
Copy link
Member

Choose a reason for hiding this comment

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

When would we get endpoint == ""? What should happen in that situation?

Copy link
Member Author

Choose a reason for hiding this comment

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

This endpoint is from config Metrics.TCPAddr.Endpoint, it's an optional parameter so it can be left unspecified (even the official doc doesn't specify it: https://docs.datadoghq.com/opentelemetry/otel_collector_datadog_exporter/?tab=onahost#setting-up-the-otel-collector-with-the-datadog-exporter).

If it's empty, the datadog exporter should use the default metric endpoint from datadog.NewConfiguration(): https://github.com/DataDog/datadog-api-client-go/blob/206cab2100541c0ed9562e8cca96394b707cbf5c/api/datadog/configuration.go#L102. We don't want to override that value with empty.

Copy link
Member

@mx-psi mx-psi Jan 4, 2023

Choose a reason for hiding this comment

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

If left unspecified, the endpoint will be set based on the site

// If an endpoint is not explicitly set, override it based on the site.
if !configMap.IsSet("metrics::endpoint") {
c.Metrics.TCPAddr.Endpoint = fmt.Sprintf("https://api.%s", c.API.Site)
}
if !configMap.IsSet("traces::endpoint") {
c.Traces.TCPAddr.Endpoint = fmt.Sprintf("https://trace.agent.%s", c.API.Site)
}
if !configMap.IsSet("logs::endpoint") {
c.Logs.TCPAddr.Endpoint = fmt.Sprintf("https://http-intake.logs.%s", c.API.Site)
}
, so I don't think we would get this in practice, unless someone explicitly sets endpoint: "".

I have a slight preference to make the whole thing fail if the endpoint is empty, but since I don't feel strongly I will approve this and merge after you see this if you think it's better this way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, I'd prefer to do the validation in a follow-up PR since that's a fairly independent change.

exporter/datadogexporter/internal/clientutil/api.go Outdated Show resolved Hide resolved
exporter/datadogexporter/internal/testutil/test_utils.go Outdated Show resolved Hide resolved
exporter/datadogexporter/metrics_exporter.go Outdated Show resolved Hide resolved
@songy23 songy23 force-pushed the ddog-metric-export branch from 005cfa2 to 3fc108c Compare January 3, 2023 21:54
configuration.UserAgent = UserAgent(buildInfo)
configuration.HTTPClient = NewHTTPClient(settings, insecureSkipVerify)
configuration.Compress = true
if endpoint != "" {
Copy link
Member

@mx-psi mx-psi Jan 4, 2023

Choose a reason for hiding this comment

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

If left unspecified, the endpoint will be set based on the site

// If an endpoint is not explicitly set, override it based on the site.
if !configMap.IsSet("metrics::endpoint") {
c.Metrics.TCPAddr.Endpoint = fmt.Sprintf("https://api.%s", c.API.Site)
}
if !configMap.IsSet("traces::endpoint") {
c.Traces.TCPAddr.Endpoint = fmt.Sprintf("https://trace.agent.%s", c.API.Site)
}
if !configMap.IsSet("logs::endpoint") {
c.Logs.TCPAddr.Endpoint = fmt.Sprintf("https://http-intake.logs.%s", c.API.Site)
}
, so I don't think we would get this in practice, unless someone explicitly sets endpoint: "".

I have a slight preference to make the whole thing fail if the endpoint is empty, but since I don't feel strongly I will approve this and merge after you see this if you think it's better this way.

@mx-psi mx-psi merged commit a052c88 into open-telemetry:main Jan 4, 2023
@songy23 songy23 deleted the ddog-metric-export branch January 4, 2023 14:50
@songy23
Copy link
Member Author

songy23 commented Jan 4, 2023

Thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exporter/datadog Datadog components
Projects
None yet
3 participants