Add instrumentation handler to collector endpoints #2664
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
Currently we do not expose any HTTP metrics on any of the collector endpoints. This makes it harder to detect when the collector is having issues ingesting bad spans for example. This change adds a metric that reports the duration of each request and adds HTTP status and path labels to it. Signed-off-by: Dimitar Dimitrov <hi@dimitardimitrov.dev>
Signed-off-by: Dimitar Dimitrov <hi@dimitardimitrov.dev>
Signed-off-by: Dimitar Dimitrov <hi@dimitardimitrov.dev>
Signed-off-by: Dimitar Dimitrov <hi@dimitardimitrov.dev>
Signed-off-by: Dimitar Dimitrov <hi@dimitardimitrov.dev>
Codecov Report
@@ Coverage Diff @@
## master #2664 +/- ##
==========================================
+ Coverage 95.07% 95.54% +0.46%
==========================================
Files 213 214 +1
Lines 9491 9535 +44
==========================================
+ Hits 9024 9110 +86
+ Misses 389 346 -43
- Partials 78 79 +1
Continue to review full report at Codecov.
|
|
Should the codecov/patch check be passing? I can't see exactly the reason for it failing, but I'm guessing it's because of the code added in init functions, which aren't tested. Is this a problem? Also, |
yurishkuro
reviewed
Nov 28, 2020
yurishkuro
reviewed
Nov 28, 2020
There was a problem hiding this comment.
Yes, please try to make sure codecov checks are passing, because we've already fallen to a lever barely above the threshold. I am not sure what you mean by init functions, we tend to write all code as testable (i.e. not using things like package-level init).
TestReload is indeed flaky, we have a ticket open for it.
Signed-off-by: Dimitar Dimitrov <hi@dimitardimitrov.dev>
We weren't testing the starting of collector's HTTP and HTTP zipkin servers. Added sanity tests for that. Also those two handlers exited with log.Fatal, which made them hard to test; this has been changed as well Signed-off-by: Dimitar Dimitrov <hi@dimitardimitrov.dev>
Only the prometheus metrics.Factory implementaiton caches the instantiated Timers, Gauges, etc. The other implementations allocate new objects each time. We want to prevent that because this is in the hot path. This adds caching on the request duration timers. This shouldn't lead to noticeable increase in memory usage. Signed-off-by: Dimitar Dimitrov <hi@dimitardimitrov.dev>
It was possible that we get concurrent map read-write panics. Signed-off-by: Dimitar Dimitrov <hi@dimitardimitrov.dev>
yurishkuro
reviewed
Nov 29, 2020
It's more robust if we use the next available port instead of hardcoding a large port. Signed-off-by: Dimitar Dimitrov <hi@dimitardimitrov.dev>
We can use directly struct keys instead of building strings out of each struct we get. Signed-off-by: Dimitar Dimitrov <hi@dimitardimitrov.dev>
Signed-off-by: Dimitar Dimitrov <hi@dimitardimitrov.dev>
yurishkuro
reviewed
Nov 30, 2020
yurishkuro
reviewed
Nov 30, 2020
yurishkuro
reviewed
Nov 30, 2020
yurishkuro
reviewed
Nov 30, 2020
Signed-off-by: Dimitar Dimitrov <hi@dimitardimitrov.dev>
Signed-off-by: Dimitar Dimitrov <hi@dimitardimitrov.dev>
yurishkuro
reviewed
Nov 30, 2020
yurishkuro
reviewed
Nov 30, 2020
Signed-off-by: Dimitar Dimitrov <hi@dimitardimitrov.dev>
jpkrohling
reviewed
Nov 30, 2020
jpkrohling
reviewed
Nov 30, 2020
| ) | ||
|
|
||
| // test wrong port number | ||
| func TestFailToListenHttp(t *testing.T) { |
There was a problem hiding this comment.
Suggested change
| func TestFailToListenHttp(t *testing.T) { | |
| func TestFailToListenHTTP(t *testing.T) { |
| assert.EqualError(t, err, "listen tcp: address -1: invalid port") | ||
| } | ||
|
|
||
| func TestSpanCollectorHttp(t *testing.T) { |
There was a problem hiding this comment.
Suggested change
| func TestSpanCollectorHttp(t *testing.T) { | |
| func TestSpanCollectorHTTP(t *testing.T) { |
Signed-off-by: Dimitar Dimitrov <hi@dimitardimitrov.dev>
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?
Resolves #2264
Currently, we do not expose any HTTP metrics on any of the collector endpoints. This makes it harder to detect when the collector is having issues ingesting bad spans for example.
Short description of the changes
This change adds a metric that reports the duration of each request and adds HTTP status and path labels to it.
Those metrics do not include the collector's gRPC endpoints.