feat: prefix k8s attributes with source and add more k8s destination attributes #226
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
JamieDanielson
changed the title
|
I couldn't recall if there was some reason why we didn't have the same attributes for destination, so I added them all here. Because these are done as 3 separate commits, we could easily just keep the first one that adds source to the pod name and uid (if indeed we were intentionally omitting destination). The second commit adds mirrored attributes and the third just refactors. |
robbkidd
force-pushed
the
robb.update-semconv
branch
from
72e552b
to
c936f46
Compare
JamieDanielson
force-pushed
the
jamie.add-source-prefix
branch
from
4cace8a
to
d320972
Compare
JamieDanielson
force-pushed
the
jamie.add-source-prefix
branch
from
d320972
to
44d3970
Compare
|
Spending a bit of time working on tests for this! |
|
@robbkidd was going to poke at tests a bit since I'll be out for a few days - timebox and if it's too cumbersome we should remove the fake cached client and merge without tests for this for now. |
kubernetes.Clientset and fake.Clientset both implement the interface. Change the param type on this function and we won't need a separate FakeCacheK8sClient implementation.
In the previous implementation, only the destination IP would be looked up. Instead, make the function generic to lookup k8s metadata based on IP and provide an optional prefix for the attribute names set. TESTS! For the k9s metadata, too!
robbkidd
force-pushed
the
jamie.add-source-prefix
branch
from
34c04ca
to
300645f
Compare
To remember how to hold this thing for testing.
This was referenced Sep 23, 2023
MikeGoldsmith
approved these changes
Sep 23, 2023
| // Provide a prefix to prepend to the attribute names, example: "source" or "destination". | ||
| // | ||
| // If the IP address is not found in the kubernetes cache, an empty map is returned. | ||
| func GetK8sAttrsForIp(client *CachedK8sClient, ip string, prefix string) map[string]any { |
There was a problem hiding this comment.
How about offer two wrapping funcs, GetK8sAttrsForSourceIP and GetK8sAttrsForDestinationIP, which include the "source" and "destination" prefixes?
It would simplify users to just call the correct func instead of knowing what prefix to use.
There was a problem hiding this comment.
Added in 3dcf19a. Whad'ya think?
We're always looking up a source or destination IP at the moment, so provide specific functions for that. They wrap the generic k8s attribute lookup and prefixer to avoid duplicating implementation. Also, we might have other IPs to lookup and prefix differently in the future.
MikeGoldsmith
approved these changes
Sep 25, 2023
robbkidd
added a commit
that referenced
this pull request
Sep 25, 2023
## Which problem is this PR solving? Refactors event processing into an handlers package. This enables better testing of libhoney event processing. ## Short description of the changes - Move libhoney event processing into new handlers package as libhoney_event_handler - Move main_test.go to handlers package as libhoney_event_handler_test - Update libhoney test to use ctx, done pattern and pass in test event via the event channel - Refactor main.go to create handler and connect up events channel - Update TCP assembler to use ctx.Done() in processing loop to handle shutdown ## How to verify that this has the expected result Nothing external. The agent should continue to work as it is. This is a refactor PR to make maintenance easier going forward. **NOTE**: This PR is based on the branch in the following PR: - #226 --------- Co-authored-by: JamieDanielson <jamieedanielson@gmail.com> Co-authored-by: Robb Kidd <robbkidd@honeycomb.io>
robbkidd
added a commit
that referenced
this pull request
Sep 25, 2023
## Which problem is this PR solving? The cached k8s client has lookup functions to aid with providing more context to events being sent. The lookup functions are inefficient because they loop over all the available data until it finds something it thinks is correct, but may not be. The PR refactors three functions to use better lookup strategies which are both faster and more accurate. - Closes #223 ## Short description of the changes- - Create a managed index on the pod informer that uses the pod's IP as the key - Update GetPodByIPAddr to use new pod index - Create a managed index on the node informer that uses the node's name as the key - Update GetNodeForPod to new use node index - Refactor GetServiceForPod to loop through registered services and applies it's selector against the given pod's labels ## How to verify that this has the expected result Lookup functions using the cached k8s client are both faster and more accurate. **NOTE**: This PR is based on the branch in the following PR: - #226 --------- Co-authored-by: JamieDanielson <jamieedanielson@gmail.com> Co-authored-by: Robb Kidd <robbkidd@honeycomb.io>
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?
Short description of the changes
source.prefix to k8s pod name and uid to more easily differentiate from destination pod name and uidHow to verify that this has the expected result
Events include the new attributes and it's easier to differentiate source from destination.