-
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
Update docs for metadata_processors #13650
Update docs for metadata_processors #13650
Conversation
Signed-off-by: chrismark <chrismarkou92@gmail.com>
------------------------------------------------------------------------------- | ||
processors: | ||
- add_kubernetes_metadata: ~ | ||
------------------------------------------------------------------------------- |
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 think there is no need to add sample configuration, there is already some configuration examples some lines below.
------------------------------------------------------------------------------- | ||
processors: | ||
- add_docker_metadata: ~ | ||
------------------------------------------------------------------------------- |
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.
Same here, I think there is no need to add another configuration example.
metadata based on which Kubernetes pod the event originated from. Each event is | ||
annotated with: | ||
metadata based on which Kubernetes pod the event originated from. | ||
At startup it will detect an `in_cluster` environment and cache the Kubernetes related metadata. |
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 think that we should more explicitly say that it will get Kubernetes-related metadata only if it finds a working configuration. Same thing for docker.
Signed-off-by: chrismark <chrismarkou92@gmail.com>
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.
👍
Shall we wait for Marjorie to have a look? @Titch990 |
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.
Looks fine, apart from these tiny things, but they seem to affect almost every line! I think I've picked up everything consistently across all the lines, but do check.
You could also consider using consistent phrasing in the Kubernetes and Docker sections where the functionality is equivalent (I know there are some differences too). If you do that, either set of words is OK.
metadata based on which Kubernetes pod the event originated from. Each event is | ||
annotated with: | ||
metadata based on which Kubernetes pod the event originated from. | ||
At startup it will detect an `in_cluster` environment and cache the |
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.
At startup it will detect an `in_cluster` environment and cache the | |
At startup it detects an `in_cluster` environment and caches the |
Present tense is usually preferred, otherwise it reads like something you haven't yet implemented and you're describing what it will do when you have!
However, perhaps it may be more accurate to say "If it detects an in_cluster
environment, it caches the Kubernetes-related metadata"? I'm not sure of your exact meaning.
metadata based on which Kubernetes pod the event originated from. | ||
At startup it will detect an `in_cluster` environment and cache the | ||
Kubernetes related metadata. If it's not able to detect a valid Kuberentes | ||
configuration, the events will not be annotated with Kubernetes related |
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.
configuration, the events will not be annotated with Kubernetes related | |
configuration, the events are not annotated with Kubernetes-related |
And again . . .
At startup it will detect an `in_cluster` environment and cache the | ||
Kubernetes related metadata. If it's not able to detect a valid Kuberentes | ||
configuration, the events will not be annotated with Kubernetes related | ||
metadata. Events will be annotated only if a valid configuration was detected. |
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.
metadata. Events will be annotated only if a valid configuration was detected. | |
metadata. Events are annotated only if a valid configuration is detected. |
I'd also put the positive statement first "Events are only annotated if a valid configuration is detected",, then what happens if not.
@@ -1360,7 +1365,11 @@ case you want to specify your own. | |||
=== Add Docker metadata | |||
|
|||
The `add_docker_metadata` processor annotates each event with relevant metadata | |||
from Docker containers: | |||
from Docker containers. At startup it will detect a docker environment and cache the metadata. |
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.
from Docker containers. At startup it will detect a docker environment and cache the metadata. | |
from Docker containers. At startup it detects a Docker environment and caches the metadata. |
from Docker containers: | ||
from Docker containers. At startup it will detect a docker environment and cache the metadata. | ||
The events will be annotated with Docker metadata, only if a valid configuration | ||
was detected and the processor is able to reach Docker api. |
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.
was detected and the processor is able to reach Docker api. | |
is detected and the processor is able to reach Docker API. |
Not
@@ -1360,7 +1365,11 @@ case you want to specify your own. | |||
=== Add Docker metadata | |||
|
|||
The `add_docker_metadata` processor annotates each event with relevant metadata | |||
from Docker containers: | |||
from Docker containers. At startup it will detect a docker environment and cache the metadata. | |||
The events will be annotated with Docker metadata, only if a valid configuration |
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 events will be annotated with Docker metadata, only if a valid configuration | |
The events are annotated with Docker metadata, only if a valid configuration |
annotated with: | ||
metadata based on which Kubernetes pod the event originated from. | ||
At startup it will detect an `in_cluster` environment and cache the | ||
Kubernetes related metadata. If it's not able to detect a valid Kuberentes |
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.
Kubernetes related metadata. If it's not able to detect a valid Kuberentes | |
Kubernetes-related metadata. If it's not able to detect a valid Kubernetes |
Signed-off-by: chrismark <chrismarkou92@gmail.com>
Thank you for your suggestions Marjorie! I think all of them were addressed. Could you give a final confirmation? @Titch990 |
LGTM! |
This is a follow-up for the PRs that enabled auto-detection for
add_docker_metadata
andadd_kubernetes_metadata
.Part of: #13068
Related to:
cc: @odacremolbap , @jsoriano