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

Update docs for metadata_processors #13650

Merged
merged 3 commits into from
Sep 19, 2019

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Sep 12, 2019

This is a follow-up for the PRs that enabled auto-detection for add_docker_metadata and add_kubernetes_metadata.

Part of: #13068
Related to:

cc: @odacremolbap , @jsoriano

Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark added containers Related to containers use case [zube]: In Review Team:Integrations Label for the Integrations team labels Sep 12, 2019
@ChrsMark ChrsMark self-assigned this Sep 12, 2019
@ChrsMark ChrsMark added the docs label Sep 12, 2019
-------------------------------------------------------------------------------
processors:
- add_kubernetes_metadata: ~
-------------------------------------------------------------------------------
Copy link
Member

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: ~
-------------------------------------------------------------------------------
Copy link
Member

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.
Copy link
Member

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>
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

👍

@ChrsMark
Copy link
Member Author

Shall we wait for Marjorie to have a look? @Titch990

Copy link
Contributor

@Titch990 Titch990 left a 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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>
@ChrsMark
Copy link
Member Author

Thank you for your suggestions Marjorie! I think all of them were addressed. Could you give a final confirmation? @Titch990

@Titch990
Copy link
Contributor

LGTM!

@ChrsMark ChrsMark merged commit e598d6c into elastic:master Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
containers Related to containers use case docs Team:Integrations Label for the Integrations team v7.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants