Add support for universal service tagging#139
Add support for universal service tagging#139ogaca-dd merged 4 commits intoDataDog:masterfrom kevingosse:kevin/tagging
Conversation
| StatsdMaxUDPPacketSize = DefaultStatsdMaxUDPPacketSize; | ||
| Advanced = new AdvancedStatsConfig(); | ||
|
|
||
| Service = System.Environment.GetEnvironmentVariable(ServiceEnvVar); |
There was a problem hiding this comment.
I'm not sure how defensive we need to be about GetEnvironmentVariable calls throwing a SecurityException since I see many other of these calls in the codebase that are not wrapped in try/catch blocks, but that came to mind when I read this block
| { | ||
| Environment.SetEnvironmentVariable("DD_ENTITY_ID", "foobar"); | ||
| using (var nonStaticServiceInstance = new DogStatsdService()) | ||
| Environment.SetEnvironmentVariable(envVar, "foobar"); |
There was a problem hiding this comment.
We should be careful about static data like these affecting other tests running at the same time, but we don't have a good way to fake env vars here, do we? (non-blocking)
There was a problem hiding this comment.
Yes it is an issue.
I have not noticed flaky tests because of that so I think we can ignore it. If I notice something, I will mock Environment.GetEnvironmentVariable.
Co-authored-by: Lucas Pimentel-Ordyna <[email protected]>
ogaca-dd
left a comment
ogaca-dd
left a comment
There was a problem hiding this comment.
Great PR 👍 , just a few minor comments.
It worths reading all the comments before starting any changes.
| _prefix = prefix; | ||
| _optionalTelemetry = optionalTelemetry; | ||
|
|
||
| var environmentTags = new List<string>(); |
There was a problem hiding this comment.
Statsd is deprecated and will be removed soon (Not thread safe, slow, does not support Windows Named pipe).
Note: You can keep the changes in the file, no need to revert them.
| { | ||
| Environment.SetEnvironmentVariable("DD_ENTITY_ID", "foobar"); | ||
| using (var nonStaticServiceInstance = new DogStatsdService()) | ||
| Environment.SetEnvironmentVariable(envVar, "foobar"); |
There was a problem hiding this comment.
Yes it is an issue.
I have not noticed flaky tests because of that so I think we can ignore it. If I notice something, I will mock Environment.GetEnvironmentVariable.
| using StatsdClient; | ||
|
|
||
| [SetUpFixture] | ||
| public class GlobalSetup |
There was a problem hiding this comment.
Is GlobalSetup needed as setting the env var is done in a single function Setting_tag_with_env_arg?
Note: If Environment.GetEnvironmentVariable starts to be annoying in the unit tests, I will mock Environment.GetEnvironmentVariable.
There was a problem hiding this comment.
This is not strictly needed, but remember that the statsd client now automatically populates tags from environment variables. On my machine I have the DD_ENV variable set, and so completely unrelated tests started failing because of the unexpected extra tag. The GlobalSetup is not needed for the CI, but this is a convenience for developers running tests locally.
There was a problem hiding this comment.
It makes sense!
Note: _values and public void TearDown() can be removed (I can do it later).
| using StatsdClient; | ||
|
|
||
| [SetUpFixture] | ||
| public class GlobalSetup |
There was a problem hiding this comment.
It makes sense!
Note: _values and public void TearDown() can be removed (I can do it later).
5 participants
Read the DD_SERVICE, DD_ENV, and DD_VERSION environment variables and set the tags appropriately.