-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expand Result to Include Signal Types #409
Merged
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
elastic-observability-automation
bot
added
the
safe-to-test
Changes are safe to run in the CI
label
Dec 6, 2024
rubvs
commented
Dec 6, 2024
marclop
reviewed
Dec 9, 2024
1pkg
reviewed
Dec 10, 2024
1pkg
previously approved these changes
Dec 10, 2024
marclop
previously approved these changes
Dec 11, 2024
marclop
approved these changes
Dec 11, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
1pkg
approved these changes
Dec 11, 2024
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.
Related to solving https://github.com/elastic/apm-managed-service/issues/606 for incoming APM events (not needed for OTEL).
Context
We need to encode the incoming signal type into the
Result
datastructure, which we can then use within ingest service and index service to extend the necessary metric attributes.Fundamentally, Result is an abstraction that encodes information after we have processed a set of events, called a batch. It operates on the level of a batch not an event.
Therefore, we don't have information on the event level. We need to update
Result
to encode signal types. There are 3 ways of doing this:Accepted
, and related, fields to a map datastructureSignal
struct, or something similar, to containerize dataSolution
Updating the current fields (1) will introduce breaking changes to both APM Server and MIS, since
Result
operates on the protocol level. Creating a new struct (2) is redundant, but I"m open to suggestions. Therefore, manually adding fields (3) is the simplest change.