Add Datadog metrics exporter #900
Conversation
* Add support for basic configuration options * Documents configuration options
MetricsEndpoint was renamed to MetricsURL
Initial metrics exporter through DogStatsD with support for all metric types but summary and distribution
* Refactor configuration * Implement telemetry and tags configuration handling * Update example configuration and README file Co-authored-by: Kylian Serrania <[email protected]>
The collector was updated to 0.9.0 upstream
* Add support for summary metric type * Add support for distribution metrics * Refactor metrics construction - Drop name in Metrics (now they act as Metric values) - Refactor constructor so that errors happen at compile-time * Report Summary total sum and count values Snapshot values are not filled in by OpenTelemetry * Report p00 and p100 as `.min` and `.max` This is more similar to what we do for our own non-additive type * Keep hostname if it has not been overridden
These are handled by the statsd package. OpenTelemetry docs are confusing and the default configuration (disabled) is different from the one returned by "GetDefault..." functions
The API exporter will be added on a future PR
Codecov Report
@@ Coverage Diff @@
## master #900 +/- ##
===========================================
+ Coverage 71.28% 88.26% +16.97%
===========================================
Files 15 238 +223
Lines 606 12660 +12054
===========================================
+ Hits 432 11174 +10742
- Misses 150 1133 +983
- Partials 24 353 +329
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
Remove unnecessary code on factory.go
|
@mx-psi do try and improve coverage a bit. |
- Remove configuration sanitization (currently unused) - Remove metrics mapping error (currently unused) - Add unit test for invalid configuration - Add unit test for invalid exporter
Necessary to keep in line with other exporters after #953 was merged
|
👋 Who else should review this before it can be merged? @asuresh4 (since you are assigned)? |
Did a quick pass through the PR on Friday and I think it looks good. I plan on taking a closer look today. |
|
@tylerbenson as one of the DataDog person can you review this? Or anyone else from DataDog can do it? |
| } | ||
|
|
||
| // CreateMetricsExporter creates a metrics exporter based on this config. | ||
| func CreateMetricsExporter( |
There was a problem hiding this comment.
This does not need to be public
|
I'll ask around to find someone that can review. |
|
@tylerbenson @mx-psi @bogdandrutu @albertvaka Hey friends. Just chiming in here, @mx-psi @tylerbenson and @albertvaka are all colleagues of mine at Datadog. I'm an approver on opentelemetry-ruby but am quite new to the collector, so I don't think I can add much to the code review, however generally I think this looks good to me. I know that some of the metrics folks have been reviewing things over on our fork and so I have a reasonable level of confidence here that Datadog as an org is both comfortable with this exporter and also plans to support it and add to it over time, should any issues arise. Additionally, I know there are a few limitations currently between what dogstatsd allows us to do and what our metric api / otel metrics permit, so while this PR might not have full coverage for every metric type, I think Datadog plans to add additional (ie agentless) export options in the future, along with (ideally) exporters for traces at the collector level. I'll try to pop in on the SIG mtg later to say hello as I have a few other unrelated questions. I hope that helps! certainly please think of @mx-psi as the voice of reason here for all things datadog metrics going forward |
There was a problem hiding this comment.
There are some reviews not addressed. Can you also split this based on https://github.com/open-telemetry/opentelemetry-collector/blob/master/CONTRIBUTING.md#when-adding-a-new-component
| // Endpoint is the DogStatsD address. | ||
| // The default value is 127.0.0.1:8125 | ||
| // A Unix address is supported | ||
| Endpoint string `mapstructure:"endpoint"` |
There was a problem hiding this comment.
There was a problem hiding this comment.
The statsd library picks up which protocol to use automatically (udp or unixgram) based on the endpoint field shape, so the transport field would have to be ignored in some cases (in particular, if endpoint starts with unix:// it's considered UDS, and if certain environment variables are set it would be UDP). Would that behavior be okay you? Otherwise we would have to leave it as it is right now.
Sure! We will address the remaining comments next week and split it up into different PRs following the recently updated contributing guidelines. Is there any other recent change we should take into account for next week? I have tried to keep everything up to date with the rest of the exporters, but it's a bit hard to keep up with all the changes going on in the Collector.
As Eric stated in his message, this has already been reviewed by Datadog folks over at our fork; I will ask internally for people to chime in and give a final review for this/separate PRs next week. |
|
Hi all, after some internal discussion we have decided to rework this PR a bit so I am closing this. I will open new PRs in the future following the contribution guidelines with the reworked configuration and implementation. Thanks for the reviews, I will take them into account for the future PRs! |
Results of running BenchmarkProtoBatchToInternalTraces: Before: BenchmarkProtoBatchToInternalTraces-12 329313 3346 ns/op 3352 B/op 43 allocs/op After: BenchmarkProtoBatchToInternalTraces-12 646017 1737 ns/op 2064 B/op 32 allocs/op 6e144e
* Extend semantic convetions for RPC * Update changelog * make service attribute conform to spec * Update api/standard/trace.go Co-authored-by: Tyler Yahn <[email protected]> Co-authored-by: Tyler Yahn <[email protected]>
* Update grpctrace instrumentation span names Span names MUST not contain the leading slash (`/`) that the grpc package prepends to all `FullMethod` values. This replaces the `serviceFromFullMethod` function with a parsing function. This parsing function returns an span name adhering to the OpenTelemetry semantic conventions as well as formatted span attributes. Additionally, the service name needs to include the package if one exists. This updates that attribute accordingly. Once #900 is merged the method attributes can be added by uncommenting. Resolves #916 * Update Changelog * Update comment to plural * Switch from regexp to string parsing * Consolidate attributes before creating span * Update Changelog with addition of rpc.method in grpctrace * Fix test spanMap lookup key * Update instrumentation/grpctrace/interceptor.go Co-authored-by: ET <[email protected]> * Unify on explicit typed return value * Fix copy paste error Co-authored-by: ET <[email protected]>
Description:
statsdpackage.Link to tracking Issue: n/a
Testing:
Documentation: