-
Notifications
You must be signed in to change notification settings - Fork 62
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
Add environment variables support for Client configuration #78
Conversation
Support of 3 environment variables for Client configuration: - "DD_AGENT_HOST" used to provide the Agent hostname. - "DD_DOGSTATSD_PORT" used to provide the DogStatsD port. - "DD_ENTITY_ID" used to provide an identifier to the Client.
9457f02
to
a4194ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments regarding the amount of (avoidable) null checks in the code.
src/StatsdClient/Statsd.cs
Outdated
@@ -179,11 +207,32 @@ private static string EscapeContent(string content) | |||
.Replace("\n", "\\n"); | |||
} | |||
|
|||
private static string ConcatTags(ref string[] constantTags,ref string[] tags) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The references to constantTags
and tags
are never set in this method, so there's no need for the ref
keyword. Arrays are always passed by reference, if that was the intent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thank you for the info
@@ -179,11 +207,32 @@ private static string EscapeContent(string content) | |||
.Replace("\n", "\\n"); | |||
} | |||
|
|||
private static string ConcatTags(ref string[] constantTags,ref string[] tags) | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it we can make the code around tags simpler by avoiding null arrays entirely. For example, early in the GetCommand()
overloads, we can replace null arrays with empty ones.
// in Statsd class
private static readonly string[] EmptyArray = new string[0];
// in GetCommand()
constantTags = constantTags ?? EmptyArray;
tags = tags ?? EmptyArray;
If we guarantee that the constantTags
and tags
arrays are never null, we wouldn't need IsNullOrEmptyT<T>()
and ConcatTags()
becomes much simpler:
private static string ConcatTags(string[] constantTags, string[] tags)
{
// concatenate all tags into a single enumerable
var allTags = constantTags.Concat(tags);
return "|#" + string.Join(",", allTags);
}
(edit: add a global empty array to avoid multiple allocations)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took your remarks, and update the PR.
src/StatsdClient/Statsd.cs
Outdated
} | ||
else | ||
{ | ||
Array.Resize(ref _constantTags, _constantTags.Length + 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to use List<T>
if we plan on resizing the collections. Then this code becomes simply:
_constantTags.Add(entityIDTag);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/StatsdClient/StatsdUDP.cs
Outdated
Port = port; | ||
if (Port == -1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zero is more idiomatic as the "not set" value here, unless -1
is already defined elsewhere. Alternatively, we should check for both, or anything less than or equal to zero.
if (Port == -1) | |
if (Port == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
d00af7b
to
1389f81
Compare
@lucaspimentel Thanks the review. |
705f939
to
8c21c96
Compare
8c21c96
to
445f89c
Compare
revert back from lists to arrays
@lucaspimentel any plan for merging it? |
@clamoriniere I'm not a maintainer on this repo or have write access. I think @yannmh can do it. Sorry about the confusion. |
Can we get a nuget release for this? The ConstantTags would be HUGE. |
@lemkepf we are on it. ETA is before the week. |
Support of 3 environment variables for Client configuration: