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
New beat info: ephemeral_id #6028
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
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
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
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.
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.
I've looked through the conditionals here, do we need a special logic for that value?
On the first run, the value of the variable s for the UUID won't be found in the delta.Strings so it will be added and the next runs will never change that value.
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.
Actually, we don't need that change because we never do delta on string we just need to return the current value we don't need to add any special logic to handle strings constants, because once they are set they will be immutable unless we manually change that value.
This is good finding @kvch! when we refactor the gauge we will also need an Immutable gauge type or something similar.
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.
even if not necessary we might want to add it to the gauge list defined in this file.
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.
My solution is temporary, until gauges are refactored. I created a separate list, because it is not really a gauge, but a string constant. I am not sure what changes you are requesting.
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.
My point was, adding this code will result in the same behavior as before. The code present in the if and the else if part of the conditionals are exactly the same. I was challenging the need to add this logic and the current need to differentiate a static gauge and a strconstant before we have a concrete type in our system that will take care of the inherent difference between the two.
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.
The first if checks if the value whether the value is constant or not. If it is constant, it is added to the
delta.Strings
list. The else if part checks if it is the first time it shows up in the string metrics or it is changed since the last sample, it is also added. If it hasn't changed since the last sample it does not show up.In other words the first if checks if the string is in
strConsts
. The second if checks if the string was in the previous sample or if it has changed since the previous sample. Adding this code definitely changes the behavior of a Beat, as before I added this codeephemeral_id
showed up only in the first event. The events after that did not include this info.I don't want to mix string constants into gauges. Gauges go into a different lists which include numerical values
delta.Ints
ordelta.Floats
. Different types of metrics are already handled differently here, seedelta.Ints
,delta.Floats and
delta.Strings`.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.
Good point