Ported wavefront-java-sdk to C# #1
Conversation
README.md
Outdated
| builder.TracingPort(tracingPort); | ||
|
|
||
| /* set this if you want to change the default flush interval of 5 seconds */ | ||
| builder.FlushIntervalSeconds(2); |
There was a problem hiding this comment.
Let's encourage users to set it higher not lower :) Maybe 30 seconds in this example.
| [Fact] | ||
| public void TestSanitize() | ||
| { | ||
| Assert.Equal("\"hello\"", Utils.Sanitize("hello")); |
There was a problem hiding this comment.
if you have a need to escape strings you can use string literals
@"""hello""" though if it is more clear is up for debate.
| /// <summary> | ||
| /// Use this format to send metric data to Wavefront. | ||
| /// </summary> | ||
| public static readonly string WavefrontMetricFormat = "wavefront"; |
There was a problem hiding this comment.
public const string WavefrontMetricFormat faster as it gets compiled down into a literal within the code; however, does have implications in how the clients consume our library (in that they would have to rebuild their clients with our version if we changed the value of our const). If the value is never expected to change this may not be an issue.
Wavefront.CSharp.SDK/Common/Utils.cs
Outdated
| } | ||
| if (String.IsNullOrWhiteSpace(tag.Key)) | ||
| { | ||
| throw new ArgumentException("metric point tag value cannot be blank"); |
There was a problem hiding this comment.
our proxies do allow this though. they silently ignore it.
There was a problem hiding this comment.
We have to keep in mind that we using the same logic for both proxy and direct ingestion.
While it is true that proxy has some safeguards for this, generally good idea to throw in SDK itself so that client send us clean data.
We do the same in our java sdk - https://github.com/wavefrontHQ/wavefront-java-sdk/blob/bc7524d86561547ecd188076a6ced75724e44a9e/src/main/java/com/wavefront/sdk/common/Utils.java#L69
| { | ||
| } | ||
|
|
||
| var statusCode = (int)HttpStatusCode.BadRequest; |
There was a problem hiding this comment.
var vs. just using int, when the type is obvious it's useful, but the type is only obvious with c-style casting? I'd favor just using int explicitly vs var, which is incredibly useful for more complex object types.
| Logger.Log(LogLevel.Warning, "Could not close old socket.", e); | ||
| } | ||
| socketOutputStream = new BufferedStream(client.GetStream(), bufferSize); | ||
| Logger.Log(LogLevel.Information, String.Format("Successfully reset connection to {0}:{1}", host, port)); |
There was a problem hiding this comment.
Nit: try to stick to 100 char limit so that I don't have to scroll right while going through the Pull Request.
Wavefront.CSharp.SDK/Common/Utils.cs
Outdated
| } | ||
| if (String.IsNullOrWhiteSpace(tag.Key)) | ||
| { | ||
| throw new ArgumentException("metric point tag value cannot be blank"); |
There was a problem hiding this comment.
We have to keep in mind that we using the same logic for both proxy and direct ingestion.
While it is true that proxy has some safeguards for this, generally good idea to throw in SDK itself so that client send us clean data.
We do the same in our java sdk - https://github.com/wavefrontHQ/wavefront-java-sdk/blob/bc7524d86561547ecd188076a6ced75724e44a9e/src/main/java/com/wavefront/sdk/common/Utils.java#L69
Wavefront.CSharp.SDK/Common/Utils.cs
Outdated
| { | ||
| throw new ArgumentException("metric point tag key cannot be blank"); | ||
| } | ||
| if (String.IsNullOrWhiteSpace(tag.Key)) |
There was a problem hiding this comment.
@hanwavefront - Shouldn't this be
if (String.IsNullOrWhiteSpace(tag.Value))
Wavefront.CSharp.SDK/Common/Utils.cs
Outdated
| { | ||
| throw new ArgumentException("histogram tag key cannot be blank"); | ||
| } | ||
| if (String.IsNullOrWhiteSpace(tag.Key)) |
There was a problem hiding this comment.
Shouldn't this be
if (String.IsNullOrWhiteSpace(tag.Value))
| /// <summary> | ||
| /// Wavefront direct ingestion client that sends data directly to Wavefront cluster via the direct ingestion API. | ||
| /// </summary> | ||
| public class WavefrontDirectIngestionClient : IWavefrontMetricSender, IWavefrontHistogramSender, IWavefrontTracingSpanSender, IBufferFlusher |
There was a problem hiding this comment.
Nit: avoid scrolling while reading github Pull requests, break into 2 lines?
| /// Client that sends data directly via TCP to the Wavefront Proxy Agent. User should probably attempt to reconnect | ||
| /// when exceptions are thrown from any methods. | ||
| /// </summary> | ||
| public class WavefrontProxyClient : IWavefrontMetricSender, IWavefrontHistogramSender, IWavefrontTracingSpanSender, IBufferFlusher |
There was a problem hiding this comment.
break into 2 lines?
| namespace Wavefront.CSharp.SDK.Integrations | ||
| { | ||
| /// <summary> | ||
| /// Client that sends data directly via TCP to the Wavefront Proxy Agent. User should probably attempt to reconnect |
There was a problem hiding this comment.
break into 2 lines to avoid scrolling to the right while reading code?
No description provided.