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

ddtrace/tracer: send all global tags instead of just env #673

Merged
merged 1 commit into from Jun 9, 2020
Merged

ddtrace/tracer: send all global tags instead of just env #673

merged 1 commit into from Jun 9, 2020

Conversation

DarrenTsung
Copy link
Contributor

Instead of only looking for Environment tag, send all global tags
for metrics recorded by the statsd client (runtime metrics).

This also removes the remapping of ext.Environment to hardcoded "env".

Fixes: #671

@DarrenTsung DarrenTsung mentioned this pull request Jun 3, 2020
Closed
@AlexJF AlexJF modified the milestone: 1.25.0 Jun 4, 2020
gbbr
gbbr reviewed Jun 4, 2020
View reviewed changes
ddtrace/tracer/option.go Outdated Show resolved Hide resolved
ddtrace/tracer/option.go Outdated Show resolved Hide resolved
ddtrace/tracer/option.go Show resolved Hide resolved
gbbr
gbbr reviewed Jun 5, 2020
View reviewed changes
Copy link
Contributor

@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.

We can have this change, please address all the comments and happy to review again and merge.

ddtrace/tracer/option_test.go Outdated Show resolved Hide resolved
@DarrenTsung
Copy link
Contributor Author

@gbbr - your comments should be addressed now 👍

gbbr
gbbr previously approved these changes Jun 5, 2020
View reviewed changes
Copy link
Contributor

@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.

This looks good to me.

@knusbaum can you also please take a look to make sure the right precedence is given to these tags based on other config? I know you've made changes in that area lately.

@gbbr gbbr requested a review from knusbaum June 5, 2020 15:41
@gbbr
Copy link
Contributor

gbbr commented Jun 5, 2020

@DarrenTsung you'll have to rebase your PR to latest v1 branch to get CI to pass.

@DarrenTsung
Copy link
Contributor Author

Seeing an error for TestPayloadIntegrity:

==================
--- FAIL: TestPayloadIntegrity (12.60s)
    --- PASS: TestPayloadIntegrity/10 (0.00s)
    --- PASS: TestPayloadIntegrity/1024 (0.10s)
    --- FAIL: TestPayloadIntegrity/131072 (12.49s)
        testing.go:853: race detected during execution of test
    testing.go:853: race detected during execution of test

Because of a race, but this doesn't seem like it would be caused by this PR?

@gbbr gbbr added this to the 1.25.0 milestone Jun 5, 2020
@gbbr
Copy link
Contributor

gbbr commented Jun 5, 2020 •
edited

Your test is reading the config

Previous read at 0x00c00017cd50 by goroutine 180:
...
gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.TestGlobalTag.func1()
      /home/circleci/dd-trace-go.v1/ddtrace/tracer/option_test.go:202 +0x141

While the statsd client attempts to send metrics:

Write at 0x00c00017cd50 by goroutine 185:
...
  github.com/DataDog/datadog-go/statsd.(*Client).Count()
      /home/circleci/go/pkg/mod/github.com/!data!dog/datadog-go@v3.7.1+incompatible/statsd/statsd.go:500 +0x69
  github.com/DataDog/datadog-go/statsd.(*Client).Incr()
      /home/circleci/go/pkg/mod/github.com/!data!dog/datadog-go@v3.7.1+incompatible/statsd/statsd.go:532 +0x92
  gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.(*tracer).worker()
      /home/circleci/dd-trace-go.v1/ddtrace/tracer/tracer.go:215 +0x339
  gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer.newTracer.func2()
      /home/circleci/dd-trace-go.v1/ddtrace/tracer/tracer.go:193 +0xdc

@gbbr
Copy link
Contributor

gbbr commented Jun 5, 2020

Best way is to just test statsTags and create a TestStatsTags. Should be enough, we don't need to test the actual datadog-go library.

@DarrenTsung
Copy link
Contributor Author

@gbbr - thanks, updated the test to just test statsTags

gbbr
gbbr reviewed Jun 5, 2020
View reviewed changes
ddtrace/tracer/option_test.go Outdated Show resolved Hide resolved
ddtrace/tracer/option_test.go Outdated Show resolved Hide resolved
@DarrenTsung
ddtrace/tracer: send all global tags instead of just env
4dcc7ef 
Instead of only looking for Environment tag, send all global tags
for metrics recorded by the statsd client (runtime metrics).

This also removes the remapping of ext.Environment to hardcoded "env".

Fixes: DataDog#671
gbbr
gbbr approved these changes Jun 5, 2020
View reviewed changes
Copy link
Contributor

@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.

Thanks!

knusbaum
knusbaum approved these changes Jun 5, 2020
View reviewed changes
Copy link
Contributor

@knusbaum knusbaum left a comment

Choose a reason for hiding this comment

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

I think we should add version to the tags as well, but it's not necessary to do here.

This looks good.

@gbbr gbbr merged commit e713241 into DataDog:v1 Jun 9, 2020
mingrammer pushed a commit to mingrammer/dd-trace-go that referenced this pull request Dec 22, 2020
@DarrenTsung @mingrammer
ddtrace/tracer: send all global tags instead of just env (DataDog#673)
688aa14 
Instead of only looking for Environment tag, send all global tags
for metrics recorded by the statsd client (and runtime metrics).

Fixes: DataDog#671
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knusbaum knusbaum knusbaum approved these changes

@gbbr gbbr gbbr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.25.0
Development

Successfully merging this pull request may close these issues.

ddtrace/tracer: WithGlobalTag does not apply to runtime metrics
4 participants
@DarrenTsung @gbbr @knusbaum @AlexJF