Add APM tracing for client-go requests to the Kubernetes API #5651
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
1fc2655 to
12aa428
Compare
barkbay
reviewed
May 18, 2022
barkbay
approved these changes
May 20, 2022
There was a problem hiding this comment.
LGTM
I'm not sure I'm the best person to review the RoundTrip function though, but given it seems inspired from the APM http module I think it is fine (it might be an argument for having it as a new module as you suggested).
Comment on lines
21
to
22
| // Allows an optional default transaction to be configured for requests where context cannot be controlled | ||
| // e.g. client-go's cache management |
There was a problem hiding this comment.
nit: latinism
Suggested change
| // Allows an optional default transaction to be configured for requests where context cannot be controlled | |
| // e.g. client-go's cache management | |
| // Allows an optional default transaction to be configured for requests where context cannot be controlled, | |
| // for example client-go's cache management. |
Comment on lines
+153
to
+164
| numSegments := 2 | ||
| // add a bit more context in the summary for PUT requests e.g. namespace | ||
| if req.Method == "PUT" { | ||
| numSegments = 3 | ||
| } | ||
|
|
||
| pathSegments := strings.Split(req.URL.Path, "/") | ||
| path := strings.Join(pathSegments[len(pathSegments)-numSegments:], "/") | ||
| // let's call out watch requests explicitly | ||
| if watch := req.URL.Query().Get("watch"); watch == "true" { | ||
| statement = "WATCH" | ||
| } |
There was a problem hiding this comment.
Any reason to not just use req.URL.Path?
There was a problem hiding this comment.
I thought it got very long in the UI so I was looking for something shorter that still conveyed the most relevant information.
|
Jenkins test this please
I am confused |
fantapsody
pushed a commit
to fantapsody/cloud-on-k8s
that referenced
this pull request
Feb 7, 2023
…#5651) This adds a custom APM round tripper for requests to the Kubernetes API. This can potentially be contributed to elastic/apm-agent-go as a new module. But I think it would be good to try it first in the context of the eck-operator. I structured the code in such a way to allow that if we decide it is worth contributing. Why a custom round tripper? To get a meaningful span summary that is specific to k8s. The current implementation is not perfect in this regard especially sub-resource requests are represented somewhat arbitrarily by subsections of the request path. The other thing implement here is to track requests done in the background by the client-go library to maintain the local cache. As far as I know there is no way to inject a context into the library that would enable tracing. This PR implements instead a default transaction mechanism to track those requests.
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.
Follow up to #5649
Will rebase once the first PR is merged. Relevant commits is:
15e7e87
This adds a custom APM round tripper for requests to the Kubernetes API. This can potentially be contributed to elastic/apm-agent-go as a new module. But I think it would be good to try it first in the context of the eck-operator. I structured the code in such a way to allow that if we decide it is worth contributing.
Why a custom round tripper?
To get a meaningful span summary that is specific to k8s. The current implementation is not perfect in this regard especially sub-resource requests are represented somewhat arbitrarily by subsections of the request path.
The other thing implement here is to track requests done in the background by the client-go library to maintain the local cache. As far as I know there is no way to inject a context into the library that would enable tracing. This PR implements instead a default transaction mechanism to track those requests.