-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Logging code cleanup related to Nomad auto-discovery #26498
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
4291b37
Logging and code cleanup related to Nomad auto-discover
andrewkroh 745ea72
Fix a few doc nits
andrewkroh 23b26bf
Update heartbeat log line assertion
andrewkroh 56f679d
Add complete example with discovery and add_nomad_metadata
andrewkroh ea6ba46
Fix secret_id and document minimal ACL policy
andrewkroh 7352205
Update Nomad policy
andrewkroh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
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
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
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
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
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
Oops, something went wrong.
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 have a semi-related note about this path template (sorry if slightly off-topic)- when testing out this feature I noticed that the
task.name
data was only being published on themeta.nomad
field of the event, not thenomad
field used in this example. Here's the relevant code:beats/x-pack/libbeat/autodiscover/providers/nomad/nomad.go
Lines 197 to 210 in 076e0a6
In my tests,
${data.nomad.task.name}
gave an invalid template reference error, but${data.meta.nomad.task.name}
resolved properly. Does this sound correct and if so, should all the documentation examples be updated to reflect this working usage? Or does a separate PR to fix the existing examples make more sense?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.
When testing I noted that there was a lot of duplicate information in the autodiscover events that get published by that code. But I wasn't sure what the expected behavior was. And now that you bring it up, I see that there are docs for what should be in the events at https://www.elastic.co/guide/en/beats/filebeat/current/configuration-autodiscover.html#_nomad. So based on those docs I think I could change this and fix the missing
nomad.task
fields. Can you open a separate issue please?Here's a sample document taken from the updated debug logs I added.
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.
Opened #26538.
It turns out there was some discussion around the need for this duplicate information in the original PR- see #14954 (comment).
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.
Great find. So based on that comment it sounds like we need to keep both
meta
andnomad
. But we can make sure all the documented keys are present under thenomad.
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 catch!
I think only the
nomad
fields need to be available for config templates. The meta fields are used for enrichment, but shouldn't be necessarily available for templates. We probably messed something up in the original PR, I remember going back and forth on this 😬We also have to double-check that hints based configs have the same fields available.