Skip to content

Conversation

@karenzone
Copy link
Contributor

@karenzone karenzone commented Jun 14, 2021

Adds more attributes to allow for more content differentiation between Beats and Elastic Agent

Related: #422

@karenzone
Copy link
Contributor Author

@andsel Let's review content together to make sure that content and context is correct for both beats and Elastic Agent.

Copy link
Contributor

@andsel andsel left a comment

Choose a reason for hiding this comment

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

At https://github.com/logstash-plugins/logstash-input-beats/pull/423/files#diff-cae5619b3d18ec99c5ccd0a9f6de0c6d3f53343c64692444551a7d29da6863e7R82 we have

...of the name to the Beat version...

maybe, for cases that are not covered by :plugin-uc: Beats, we should define another uppercase not plural placeholder

Copy link
Contributor

@andsel andsel left a comment

Choose a reason for hiding this comment

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

@karenzone
Copy link
Contributor Author

Carry over note from #417: #417 (review)

@karenzone karenzone force-pushed the apply-more-attributes branch from 24715fa to a160108 Compare June 16, 2021 21:00
@karenzone
Copy link
Contributor Author

@andsel Those deeplinks aren't resolving correctly for me, so I made some guesses and changed more instances.

Other place to discuss/resolve:

Noting here that you've already explained that the ECS section should use the [beats] namespace.
I'll set up some time for us to run through the remaining instances and resolve them.

@karenzone karenzone force-pushed the apply-more-attributes branch from a160108 to 871bdb7 Compare June 22, 2021 00:49
metricbeat-7.4.0.
of the metadata field and `%{[@metadata][version]}` sets the second part to
the {plugin-singular} version. For example:
metricbeat-7.4.0. or elastic_agent-7.14.0.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andsel Will this work as documented? We're relying on metadata, so I think it might not.

Copy link
Contributor

Choose a reason for hiding this comment

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

@karenzone we should remove any reference to [@metadata][beat] and [@metadata][version] because the agent doesn't send that information

@karenzone
Copy link
Contributor Author

@andsel Ready for review. Also, I left a question inline. Thanks!

@karenzone karenzone requested a review from andsel June 22, 2021 18:48
@karenzone karenzone force-pushed the apply-more-attributes branch from 871bdb7 to 8e39223 Compare June 28, 2021 17:30
@karenzone
Copy link
Contributor Author

@andsel Do we have the info we need to wrap this up?

Copy link
Contributor

@andsel andsel left a comment

Choose a reason for hiding this comment

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

HI @karenzone as described in the comment left inline, we should remove reference to metadata sent by beat in the case of agent

metricbeat-7.4.0.
of the metadata field and `%{[@metadata][version]}` sets the second part to
the {plugin-singular} version. For example:
metricbeat-7.4.0. or elastic_agent-7.14.0.
Copy link
Contributor

Choose a reason for hiding this comment

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

@karenzone we should remove any reference to [@metadata][beat] and [@metadata][version] because the agent doesn't send that information

@karenzone
Copy link
Contributor Author

@andsel Thanks for this update.

we should remove any reference to [@metadata][beat] and [@metadata][version] because the agent doesn't send that information

What should we replace it with in the examples, ILM, and versioned indices sections?

@andsel
Copy link
Contributor

andsel commented Jul 14, 2021

In that case, I think we could cut that part of ILM warning and Versioned indices, for the example we could replace it with a datastream enabled one, like:

output {
  elasticsearch {
    hosts => ["http://localhost:9200"]
    data_stream => "true"
  }
}

WDYT?

@karenzone
Copy link
Contributor Author

@andsel This approach is working so far. For the Versioned indices info, are you proposing that we remove that whole section for Agent for now?

@karenzone karenzone force-pushed the apply-more-attributes branch from a7d7322 to bfe307d Compare July 14, 2021 22:37
@karenzone karenzone force-pushed the apply-more-attributes branch from bfe307d to 22a7f8c Compare July 14, 2021 22:39
the {plugin-uc} version. For example:
metricbeat-7.4.0.
of the metadata field and `%{[@metadata][version]}` sets the second part to
the {plugin-singular} version. For example: metricbeat-7.4.0.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andsel Should this reflect the beats version (instead of the stack version)?

Copy link
Contributor

Choose a reason for hiding this comment

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

[@metadata][version] field read from the remote beats is the version of the remote beat and not of the overall stack

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for validating. I will fix.

@karenzone karenzone requested a review from andsel July 14, 2021 22:49
@karenzone
Copy link
Contributor Author

@andsel Thanks for clarifying some of the fine details. I've introduced conditional processing to handle the differences. Please let me know what you think.

Copy link
Contributor

@andsel andsel left a comment

Choose a reason for hiding this comment

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

From a content perspective it LGTM!

Maybe we should check how it renders

@karenzone
Copy link
Contributor Author

karenzone commented Jul 15, 2021

@andsel

Maybe we should check how it renders

I always do that locally as part of content development, but it would be nice for you to see it, too.

We can't generate docs until this PR is merged and the gem is published. However, I can do a close approximation. I will put that content into a PR with "do not merge" warnings so you can see it, too. Stay tuned!

UPDATE:
elastic/logstash-docs#1102

@andsel andsel self-requested a review July 15, 2021 13:43
Copy link
Contributor

@andsel andsel left a comment

Choose a reason for hiding this comment

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

LGTM

@karenzone karenzone merged commit bb56025 into logstash-plugins:master Jul 15, 2021
@karenzone karenzone deleted the apply-more-attributes branch July 15, 2021 13:47
@karenzone
Copy link
Contributor Author

Published at v6.1.6: https://rubygems.org/gems/logstash-input-beats/versions/6.1.6-java
Thanks for publishing, @kares ❤️

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.

3 participants