Skip to content
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 2 commits into from Jan 10, 2018
Merged

Conversation

kvch
Copy link
Contributor

@kvch kvch commented Jan 9, 2018

New "metric" is added: beat.info.ephemeral_id. I am open to alternative names for this.
This ID is generated every time the Beat starts/restarts. It does not change when the config is reloaded.

I also added strConsts list which contains strings which should be added to the monitoring events even if it is unchange. This solution is the same as the current gauges list.

if p, ok := prev.Strings[k]; !ok || p != s {
if _, found := strConsts[k]; found {
delta.Strings[k] = s
} else if p, ok := prev.Strings[k]; !ok || p != s {
delta.Strings[k] = s
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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 code ephemeral_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 or delta.Floats. Different types of metrics are already handled differently here, see delta.Ints, delta.Floats and delta.Strings`.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 code ephemeral_id showed up only in the first event. The events after that did not include this info.

Good point

@exekias
Copy link
Contributor

exekias commented Jan 10, 2018

Would instance_id look nicer? I'm not quite sure, It may be misleading

@ph
Copy link
Contributor

ph commented Jan 10, 2018

@exekias I think we should stay with the ephemeral_id, this is what ES and Logstash are using for the lifetime UUID.

edited, added reference
https://www.elastic.co/guide/en/elasticsearch/reference/current/cat-thread-pool.html#_other_fields

elastic/logstash#7407

@exekias
Copy link
Contributor

exekias commented Jan 10, 2018

Good point, forget about my message then 😇

Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @kvch!

@ph ph merged commit b9d1db8 into elastic:master Jan 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants