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.

Sign up for GitHub

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

Jump to bottom

Add Datadog exporter overall structure #1142

Merged
merged 17 commits into from
Sep 30, 2020
Merged

Add Datadog exporter overall structure #1142

merged 17 commits into from
Sep 30, 2020

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Sep 28, 2020 •
edited

Description:

Add Datadog exporter overall structure following the contributing guidelines.

This is a rework of #900: instead of using statsd it will report directly to Datadog's backend. It incorporates the feedback from that PR's reviews.

Link to tracking Issue: n/a

Testing: Added unit tests, tested full version (available on our fork) on an end to end testing environment.

Documentation: This adds the README for the exporter with the intended instructions for the full metrics exporter.

Note: The commit numbering refers to the PRs on our fork but are shown incorrectly by Github.

@mx-psi
Add DataDog exporter back from old fork
0345c29 
commit 99129fb
Author: Pablo Baeyens <pablo.baeyens@datadoghq.com>
Date:   Thu Sep 3 18:10:28 2020 +0200

    Handle namespace at initialization time

commit babca25
Author: Pablo Baeyens <pablo.baeyens@datadoghq.com>
Date:   Thu Sep 3 17:23:53 2020 +0200

    Initialize on a separate function
    This way the variables can be checked without worrying about the env

commit 24d0cb4
Author: Pablo Baeyens <pablo.baeyens@datadoghq.com>
Date:   Thu Sep 3 14:30:35 2020 +0200

    Check environment variables for unified service tagging

commit 6695f82
Author: Pablo Baeyens <pablo.baeyens@datadoghq.com>
Date:   Wed Sep 2 14:57:37 2020 +0200

    Add support for sending metrics through the API
    - Use datadog.Metric type for simplicity
    - Get host if unset

commit c366603
Author: Pablo Baeyens <pablo.baeyens@datadoghq.com>
Date:   Wed Sep 2 09:56:24 2020 +0200

    Disable Queue and Retry settings (#72)

    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

commit a660b56
Author: Pablo Baeyens <pablo.baeyens@datadoghq.com>
Date:   Tue Sep 1 15:26:14 2020 +0200

    Add support for summary and distribution metric types (#65)

    * 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

commit c95adc4
Author: Pablo Baeyens <pablo.baeyens@datadoghq.com>
Date:   Thu Aug 27 13:00:02 2020 +0200

    Update dependencies and `make gofmt`
    The collector was updated to 0.9.0 upstream

commit 20afb0e
Author: Pablo Baeyens <pablo.baeyens@datadoghq.com>
Date:   Wed Aug 26 18:25:49 2020 +0200

    Refactor configuration (#45)

    * Refactor configuration
    * Implement telemetry and tags configuration handling
    * Update example configuration and README file
    Co-authored-by: Kylian Serrania <kylian.serrania@datadoghq.com>

commit fdc98b5
Author: Pablo Baeyens <pablo.baeyens@datadoghq.com>
Date:   Fri Aug 21 11:09:08 2020 +0200

    Initial DogStatsD implementation (#15)

    Initial metrics exporter through DogStatsD with support for all metric types but summary and distribution

commit e848a60
Author: Pablo Baeyens <pablo.baeyens@datadoghq.com>
Date:   Fri Aug 21 10:42:45 2020 +0200

    Bump collector version

commit 58be9a4
Author: Pablo Baeyens <pablo.baeyens@datadoghq.com>
Date:   Thu Aug 6 10:04:32 2020 +0200

    Address linter

commit 695430c
Author: Pablo Baeyens <pablo.baeyens@datadoghq.com>
Date:   Tue Aug 4 13:28:01 2020 +0200

    Fix field name error

    MetricsEndpoint was renamed to MetricsURL

commit 168b319
Author: Pablo Baeyens <pablo.baeyens@datadoghq.com>
Date:   Mon Aug 3 11:05:01 2020 +0200

    Create initial outline for Datadog exporter (#1)

    * Add support for basic configuration options
    * Documents configuration options
@mx-psi
go mod tidy
dfd3cbe
@mx-psi
Address feedback from upstream PR we did not merge (#1)
0ce7520 
* Backport changes from upstream PR

Remove `err` from MapMetrics

* Remove usage of pdatautil

* Fix tests

* Use TCPAddr

* Review which functions should be private
@mx-psi
Remove DogStatsD mode (#2)
fcc9b77 
* Remove DogStatsD mode

* go mod tidy

* Remove mentions to DogStatSD
@mx-psi
Improve test coverage (#3)
53da05f 
* Improve test coverage

Added unit tests for
- API key censoring
- Hostname
- Metrics exporter

Renamed test and implementation files for consistency

* Add one additional test
@mx-psi
Remove client validation (#6)
4dd5037 
The zorkian API does not validate the API key unless you also have
an application key, even though the endpoint works without it.

I am removing this validation until this gets fixed on the zorkian library
@mx-psi
Keep only configuration and factory methods
6da1a53 
Following the contribution guidelines we need to make a first PR
with this
@mx-psi
Merge remote-tracking branch 'upstream/master' into exporter-config
a3cc23b
@mx-psi
Use latest version of collector
fb3d8e6
@mx-psi
Remove report_percentiles option
5ba5c76 
It is not being used as of now until the OTLP metrics format
stabilizes and we have a Summary type metric again
@mx-psi
Correct configuration
12e2935 
The API key is now a required setting
@project-bot project-bot bot added this to In progress in Collector Sep 28, 2020
@codecov
Copy link

codecov bot commented Sep 28, 2020 •
edited

Codecov Report

Merging #1142 into master will increase coverage by 0.01%.
The diff coverage is 95.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1142      +/-   ##
==========================================
+ Coverage   89.89%   89.91%   +0.01%     
==========================================
  Files         276      278       +2     
  Lines       13621    13667      +46     
==========================================
+ Hits        12244    12288      +44     
- Misses       1011     1012       +1     
- Partials      366      367       +1
Flag Coverage Δ
#integration 76.01% <ø> (ø)
#unit 89.06% <95.65%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
exporter/datadogexporter/config.go 90.90% <90.90%> (ø)
exporter/datadogexporter/factory.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f52fd7...18f58f8. Read the comment docs.

@mx-psi mx-psi marked this pull request as ready for review September 28, 2020 13:48
@mx-psi mx-psi requested a review from a team as a code owner September 28, 2020 13:48
@julien-lebot
Copy link
Contributor

Approving on behalf of Datadog

tigrannajaryan
tigrannajaryan approved these changes Sep 29, 2020
View reviewed changes
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM. Can be merged if other approvers don't have comments.

exporter/datadogexporter/config.go Outdated Show resolved Hide resolved
Collector automation moved this from In progress to Reviewer approved Sep 29, 2020
@tigrannajaryan tigrannajaryan self-assigned this Sep 29, 2020
@tigrannajaryan
Copy link
Member

Please resolve the conflict.

mx-psi and others added 3 commits September 30, 2020 11:25
@mx-psi
Merge branch 'master' into exporter-config
fc22042
@mx-psi @tigrannajaryan
Apply suggestions from code review
501243e 
Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
@mx-psi
Separate documentation into two examples
18f58f8 
One example with the minimal configuration, for sending to `datadoghq.com`
and a second one for sending to `datadoghq.eu`
@mx-psi
Copy link
Member Author

mx-psi commented Sep 30, 2020

@tigrannajaryan I addressed the last comments and resolved the conflict.

@bogdandrutu bogdandrutu merged commit 3e375ad into open-telemetry:master Sep 30, 2020
Collector automation moved this from Reviewer approved to Done Sep 30, 2020
@mx-psi mx-psi mentioned this pull request Oct 2, 2020
Merged
@mx-psi mx-psi mentioned this pull request Oct 27, 2020
Closed
6 tasks
dyladan referenced this pull request in dynatrace-oss-contrib/opentelemetry-collector-contrib Jan 29, 2021
@tigrannajaryan
Update OTLP to v0.4.0 [Breaking change] (#1142)
4777e91 
This is a breaking OTLP change.

- Use AnyValue introduced in recent change to OTLP.
  Changes are encapsulated in AttributeValue and most of the codebase is
  unaffected, which proves the wrappers are very useful.

- Rename AttributeKeyValue to KeyValue (the change comes from OTLP).

- Use local protoc to compile ProtoBufs. Previously used znly/protoc
  docker image is outdated and results in incorrect code for gRPC-Gateway.

TODO:

- Need to add support for ARRAY value type. This is not urgent since
  there are no known data sources that use the ARRAY type yet.

- Use Gogoproto `(gogoproto.nullable) = false` annotation for AnyValue
  to possibly improve performance further.
ljmsc referenced this pull request in ljmsc/opentelemetry-collector-contrib Feb 21, 2022
@Aneurysm9
Rename CorrelationContext to Baggage (#1142)
06689a0 
* Rename 'correlation' to 'baggage'

* Rename CorrelationContext progator to Baggage

* Update CHANGELOG
codeboten pushed a commit that referenced this pull request Nov 23, 2022
@lzchen
Use is_recording flag in asgi, pyramid, aiohttp instrumentation (#1142)
7718d88
codeboten pushed a commit that referenced this pull request Nov 23, 2022
@lzchen
Use is_recording flag in asgi, pyramid, aiohttp instrumentation (#1142)
d6c6abf
codeboten pushed a commit that referenced this pull request Nov 23, 2022
@lzchen
Use is_recording flag in asgi, pyramid, aiohttp instrumentation (#1142)
2b37136
codeboten pushed a commit that referenced this pull request Nov 23, 2022
@lzchen @codeboten
Use is_recording flag in asgi, pyramid, aiohttp instrumentation (#1142)
5f100f0
codeboten pushed a commit that referenced this pull request Nov 23, 2022
@lzchen @codeboten
Use is_recording flag in asgi, pyramid, aiohttp instrumentation (#1142)
847418f
codeboten pushed a commit that referenced this pull request Nov 23, 2022
@lzchen @codeboten
Use is_recording flag in asgi, pyramid, aiohttp instrumentation (#1142)
3fe8225
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bogdandrutu bogdandrutu bogdandrutu left review comments

@julien-lebot julien-lebot julien-lebot approved these changes

@tigrannajaryan tigrannajaryan tigrannajaryan approved these changes

Assignees

@tigrannajaryan tigrannajaryan

Labels
None yet
Projects
No open projects
Collector
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mx-psi @julien-lebot @tigrannajaryan @bogdandrutu