Append traces path to http endpoint #279
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pkanal
force-pushed
the
purvi/traces-path-http
branch
from
b745799 to
73080ba
Compare
cartermp
reviewed
Sep 27, 2022
JamieDanielson
added
the
version: bump minor
MikeGoldsmith
requested changes
Sep 28, 2022
There was a problem hiding this comment.
This looks great, thanks @pkanal 🎉
Adds some feedback and suggestions plus some nits about whitespace 😄
| @@ -95,10 +97,12 @@ public static TracerProviderBuilder AddHoneycomb(this TracerProviderBuilder buil | |||
| if (options.Debug) | |||
| { | |||
| builder.AddConsoleExporter(); | |||
| Console.WriteLine("DEBUG: HoneycombOptions"); | |||
| Console.WriteLine(JsonSerializer.Serialize(options, new JsonSerializerOptions { WriteIndented = true })); | |||
Co-authored-by: Jamie Danielson <[email protected]> Co-authored-by: Mike Goldsmith <[email protected]>
MikeGoldsmith
approved these changes
Sep 28, 2022
There was a problem hiding this comment.
Looks great, thanks @pkanal
I left one minor suggestion regarding local variable naming 😄
pkanal
added a commit
that referenced
this pull request
Sep 29, 2022
## Which problem is this PR solving? This PR follows up on #279 and adds the same logic to append `/v1/metrics` path to the endpoint if the protocol is `http/protobuf` or `http/json` ## Short description of the changes - Adds smoke tests for metrics - Consolidates tests with many cases using [`InlineData`](https://learn.microsoft.com/en-us/dotnet/core/testing/unit-testing-with-dotnet-test#add-more-tests) - Adds logic to append `/v1/metrics` to the generic endpoint if a path is not already specified
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which problem is this PR solving?
This PR adds the logic to append
/v1/tracesto the generic endpoint if the protocol specified ishttp/protobuf. It also refactors how fallback values are set and how environment values are set. This was necessary work in order to get the path to append properly for config values and env vars and overall allows us to test HoneycombOptions and EnvironmentOptions reliably.Short description of the changes
HoneycombOptionsandEnvironmentOptions. Removed fallback values and defaults altogether fromEnvironmentOptions, they should be handled in one place inHoneycombOptions.HoneycombOptionsfromEnvironmentOptions./v1/tracespath to generic endpoint if the protocol ishttp/protobufHoneycombOptionswhenDebug = true