[CONTP-388] use entityid struct in tagger component #30133
Conversation
adel121
added
kind/enhancement
component/tagger
changelog/no-changelog
team/container-platform
adel121
force-pushed
the
adelhajhassan/use_entityid_struct_in_tagger_component
branch
from
e5f3e14 to
28cdd04
Compare
adel121
changed the title
adel121
force-pushed
the
adelhajhassan/use_entityid_struct_in_tagger_component
branch
2 times, most recently
from
fdf5e74 to
825243d
Compare
Regression Detector |
adel121
force-pushed
the
adelhajhassan/use_entityid_struct_in_tagger_component
branch
from
825243d to
bffd7a1
Compare
Test changes on VMUse this command from test-infra-definitions to manually test this PR changes on a VM: inv create-vm --pipeline-id=47245178 --os-family=ubuntuNote: This applies to commit 5b8925c |
adel121
force-pushed
the
adelhajhassan/use_entityid_struct_in_tagger_component
branch
from
bffd7a1 to
d82fa97
Compare
| // If possible, avoid using this function, and use the Tag interface function instead. | ||
| // This function exists in order not to break backward compatibility with rtloader and python | ||
| // integrations using the tagger | ||
| func LegacyTag(entity string, cardinality types.TagCardinality) ([]string, error) { |
There was a problem hiding this comment.
Since AIUI this API is not going away any time soon, would it make sense to give it a descriptive name? Something like GetTagsByString.
There was a problem hiding this comment.
I see your point, but the thing here is that we need to push developers away from using this method because it includes an extra step of string parsing, which we can avoid by calling Tag method of the tagger.
This is why I think it is good to include something in the function name indicating that calling it should be avoided except in very specific cases.
How about:
LegacyTagByString ?
comp/core/tagger/global.go
Outdated
| // integrations using the tagger | ||
| func LegacyTag(entity string, cardinality types.TagCardinality) ([]string, error) { | ||
| if globalTagger == nil { | ||
| return nil, fmt.Errorf("a global tagger must be set before calling Tag") |
There was a problem hiding this comment.
fmt.Errorf is a function aiming at formatting a string.
When there’s nothing to format, it’s much more efficient to use a function that just wraps the already formatted string into an error:
| return nil, fmt.Errorf("a global tagger must be set before calling Tag") | |
| return nil, errors.New("a global tagger must be set before calling Tag") |
There was a problem hiding this comment.
Indeed you are right.
fmt.Errorf is already used in this file in many place, where it can be replaced by errors.New.
I updated all of them.
There was a problem hiding this comment.
Indeed, I noticed that this pattern was common in our code base.
I’ve also noticed that the perfsprint linter, included in latest versions of golangci-lint complains about it.
So, we’ll hopefully have to fix all of them next time we update golangci-lint.
comp/core/tagger/global.go
Outdated
|
|
||
| entityID, err := types.NewEntityIDFromString(entity) | ||
| if err != nil { | ||
| return []string{}, err |
There was a problem hiding this comment.
Does it make a difference to return an empty slice instead of nil (like what is done above line 53) in case error ?
| return []string{}, err | |
| return nil, err |
| var tags []string | ||
| if entityID != "" { | ||
| if entityID.Empty() { |
There was a problem hiding this comment.
This change looks counter-intuitive to me because I would have expected "" to be equivalent to .Empty() and not the other way round.
Shouldn’t this be:
| if entityID.Empty() { | |
| if entityID.NotEmpty() { |
or
| if entityID.Empty() { | |
| if !entityID.Empty() { |
?
| var tags []string | ||
| if entityID != "" { | ||
| if entityID.Empty() { |
There was a problem hiding this comment.
This change looks counter-intuitive to me because I would have expected "" to be equivalent to .Empty() and not the other way round.
Shouldn’t this be:
| if entityID.Empty() { | |
| if entityID.NotEmpty() { |
or
| if entityID.Empty() { | |
| if !entityID.Empty() { |
?
adel121
force-pushed
the
adelhajhassan/use_entityid_struct_in_tagger_component
branch
from
8399d27 to
e6a0deb
Compare
adel121
force-pushed
the
adelhajhassan/switch_to_composite_store_and_use_entity_id_struct
branch
from
eabaac2 to
146a410
Compare
adel121
force-pushed
the
adelhajhassan/use_entityid_struct_in_tagger_component
branch
from
2e0f6dd to
50cc5a2
Compare
|
📥 📢 Info, this pull request increases the binary size of serverless extension by 0 bytes. Each MB of binary size increase means about 10ms of additional cold start time, so this pull request would increase cold start time by 0ms. Debug infoIf you have questions, we are happy to help, come visit us in the #serverless slack channel and provide a link to this comment. We suggest you consider adding the |
Serverless Benchmark Results
tl;drUse these benchmarks as an insight tool during development.
What is this benchmarking?The The benchmark is run using a large variety of lambda request payloads. In the charts below, there is one row for each event payload type. How do I interpret these charts?The charts below comes from The benchstat docs explain how to interpret these charts.
I need more helpFirst off, do not worry if the benchmarks are failing. They are not tests. The intention is for them to be a tool for you to use during development. If you would like a hand interpreting the results come chat with us in Benchmark stats |
|
/merge |
|
🚂 MergeQueue: pull request added to the queue The median merge time in Use |
dd-mergequeue
bot
deleted the
adelhajhassan/use_entityid_struct_in_tagger_component
branch
What does this PR do?
This PR changes the signature of the
Tagmethod of the tagger interface to receive the entity id as a struct instead of a string.Motivation
TL:DR; Avoid string manipulation overhead.
Full context:
Entity ID is composed of a prefix and an id and is displayed as
{prefix}://{id}.Callers of the tagger usually have the prefix and the id parts separated, and they form the entityID as a string by concatenating the prefix and id along with the separator:
fmt.Sprintf("%s://%s", prefix, id).This entityID is passed as a string to the tagger Tag method to get the entity tags.
However, the tagger needs to separate the prefix and the id, so it will need to parse the string, adding an overhead.
To eliminate this overhead, with the change introduced in this PR, the entityID will now be passed as a struct containing 2 fields, one for the prefix and one for the id, eliminating the need to perform string manipulations (concatenations and splits).
Describe how to test/QA your changes
Unit tests and E2E tests should cover most the QA.
However, we still need to verify that python integrations can still query the tagger normally.
Here is a sample QA:
Deploy the agent with helm.
Do the following steps to deploy a custom python check onto the agent:
kubectl exec -it datadog-agent-hsstl -c agent -- bin/bashcustom_checkvalue.yamlunderetc/datadog-agent/conf.dwith the following content:custom_checkvalue.pyunderetc/datadog-agent/checks.dwith the following content:ATTENTION: you should use the entity id that you go from the tagger-list command in step 2.
agent stop && agent startWait for the agent to start, and run the custom check as follows:
Note: you migh not get the tags on each run due to lag of initialisation of workloadmeta and tagger. However, as long as you are getting the tags at least once, you can consider the QA as done. You can also check the Datadog UI to validate that you can see the metric and aggregate it by tags:
Possible Drawbacks / Trade-offs
Rtloader exposes the tagger tag method to the python integrations running in the datadog agent.
Changing the signature of the Tag method breaks the rtloader interface, and thus breaks the calls to the tagger from the python integrations.
To preserve backward compatibility in regards for rtloader, the old signature is kept in the codebase, but now with the name
LegacyTag, and the rtloader now redirects python calls to the tagger Tag method to theLegacyTagfunction.The
LegacyTagfunction receives the entityID as a string, parses the string to split it to a prefix and and an id, and redirects the call to theTagfunction.Additional Notes