Support sending a service tag for all integrations
#4877
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
ofek
added
component/collector
component/aggregator
[deprecated] team/agent-core
ofek
force-pushed
the
ofek/service-tag
branch
6 times, most recently
from
bcb5ae3
to
4f84cb0
Compare
ofek
force-pushed
the
ofek/service-tag
branch
from
4f84cb0
to
acac344
Compare
While running unit tests, we have to be careful that the Aggregator does not already contain a MockedSender, which may not behave the way we expect to and which is very confusing while looking at the test failed result.
truthbk
reviewed
Feb 12, 2020
| uptime = uptimeSampler | ||
| uptimeCheck := new(UptimeCheck) | ||
| uptimeCheck.Configure(nil, nil, "test") | ||
|
|
||
| mock := mocksender.NewMockSender(uptimeCheck.ID()) | ||
| // it's not needed but we've better to recreate it with the check ID, for the sake of correctness | ||
| mock = mocksender.NewMockSender(uptimeCheck.ID()) |
There was a problem hiding this comment.
I think I would rather have us do something like:
aggregator.SetSender(mock, uptimeCheck.ID())
or add a helper to mocksender to do it.
| fileHandleCheck := new(fhCheck) | ||
| fileHandleCheck.Configure(nil, nil, "test") | ||
|
|
||
| mock := mocksender.NewMockSender(fileHandleCheck.ID()) | ||
| // it's not needed but we've better to recreate it with the check ID, for the sake of correctness | ||
| mock = mocksender.NewMockSender(fileHandleCheck.ID()) |
| // SetCheckService appends the service as a tag for metrics, events, and service checks | ||
| // This may be called any number of times, though the only the last call will have an effect | ||
| func (s *checkSender) SetCheckService(service string) { | ||
| s.service = service |
There was a problem hiding this comment.
indeed every check will have its own sender, so this should be 👍
| // because the FinalizeCheckServiceTag is called in Configure. | ||
| // Hopefully, the check ID is an empty string while running unit tests; | ||
| mock := mocksender.NewMockSender("") | ||
| mock.On("FinalizeCheckServiceTag").Return() |
There was a problem hiding this comment.
we should maybe call mock.SetupAcceptAll() instead?
ofek
force-pushed
the
ofek/service-tag
branch
from
cc3926b
to
f107b6a
Compare
truthbk
previously approved these changes
Feb 12, 2020
ofek
force-pushed
the
ofek/service-tag
branch
from
6232d00
to
49f14ba
Compare
truthbk
approved these changes
Feb 12, 2020
This was referenced Feb 13, 2020
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.
Motivation
We want to feature the notion of service more prominently for integrations so that we can correlate metrics with logs and traces