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

[Agent] Inject index with sane defaults to configuration #16903

Merged
merged 10 commits into from
Mar 11, 2020

Conversation

michalpristas
Copy link
Contributor

Migrated from: #15852
Build on top of: #15809

Added one extra rule for injecting index into inputs based on provided type and detected namespace/dataset type.

Other option was to use decorator but at the time of decorating there are already some pieces of config missing.

Draft until final configuration format is settled and mentioned PR is in, but READY for review.

It overwrites index specified in input.

Fixes: #15690
Fixes: #15691

cc @ruflin @ph

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ingest-management (Project:fleet)

Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

I think it's important that the config we have here is the a config that already provides value to the user. It's not only a sample. I compare to to running metricbeat with ./metricbeat. All the system data is just there and works.

- /var/log/hello1.log
- /var/log/hello2.log
datasources:
- namespace: sample
Copy link
Member

Choose a reason for hiding this comment

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

Why not using the default namespace here?

- type: logs
processors:
streams:
- dataset: sample.acccess
Copy link
Member

Choose a reason for hiding this comment

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

I suggest if we don't have any good default logs here, we just skip the log parts / leave it commented out. I would suggest the system logs here (check logs system module)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

system module seems purely linux oriented so i left it out so our future windows test wont fail on something like this

Copy link
Member

Choose a reason for hiding this comment

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

That is where in the future our constraints come into play. I think part of it work also on OS X

The samples you put in will also not work. Suggestion: Lets skip the logs part for now but have a logs sample based on the Linux logs inside but commented out. So users can easily edit it.

paths: /var/log/sample/error.log
- type: system/metrics
streams:
- metricset: cpu
Copy link
Member

Choose a reason for hiding this comment

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

Lets add a few more here like memory, disk.

Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Can you change to the default namespace everywhere?

- /var/log/hello1.log
- /var/log/hello2.log
datasources:
- namespace: sample
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- namespace: sample
- namespace: default

- /var/log/hello1.log
- /var/log/hello2.log
datasources:
- namespace: sample
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- namespace: sample
- namespace: default

- /var/log/hello1.log
- /var/log/hello2.log
datasources:
- namespace: sample
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- namespace: sample
- namespace: default

go.sum Outdated
@@ -214,6 +214,7 @@ github.com/eapache/queue v1.1.0 h1:YOEu7KNc61ntiQlcEeUIoDTJ2o8mQznoNvUhiigpIqc=
github.com/eapache/queue v1.1.0/go.mod h1:6eCeP0CKFpHLu8blIFXhExK/dRa7WDZfr6jVFPTqq+I=
github.com/eclipse/paho.mqtt.golang v1.2.1-0.20200121105743-0d940dd29fd2 h1:DW6WrARxK5J+o8uAKCiACi5wy9EK1UzrsCpGBPsKHAA=
github.com/eclipse/paho.mqtt.golang v1.2.1-0.20200121105743-0d940dd29fd2/go.mod h1:H9keYFcgq3Qr5OUJm/JZI/i6U7joQ8SYLhZwfeOo6Ts=
github.com/elastic/beats v7.6.1+incompatible h1:4iPVpFr8BSJW2fUVi+/tYXQ4v/IYVx0k2PPLTg8cfks=
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is weird?

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.

code LGTM, I haven't tested it.

Test looks OK to catch the problems.

- enabled: true
metricset: info

settings.monitoring:
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for changing this.

@ph
Copy link
Contributor

ph commented Mar 10, 2020

@michalpristas lets add this change to the new changelog too, so we know when the new syntax has landed.

@michalpristas
Copy link
Contributor Author

yep i will after we merge changelog in

@ph ph added the needs_backport PR is waiting to be backported to other branches. label Mar 10, 2020
settings.monitoring:
use_output: monitoring

management:
Copy link
Member

Choose a reason for hiding this comment

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

Not in this PR, but thinking this + config should also be under settings?

inputs:
- type: system/metrics
streams:
- metricset: cpu
Copy link
Member

Choose a reason for hiding this comment

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

Had a quick chat with @michalpristas about this on Slack. The problem here is that this will send data to metrics-generic-default but it should end up in metrics-system.cpu-default. We probably have to add the dataset here at the moment and simplify it later.

Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Configs LGTM.

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, lets get that merged in.

@ph
Copy link
Contributor

ph commented Mar 11, 2020

I've added the "need_backport" will merge after #16885

@michalpristas michalpristas self-assigned this Mar 11, 2020
@michalpristas
Copy link
Contributor Author

Jenkins test this

@michalpristas michalpristas merged commit 96e4506 into elastic:master Mar 11, 2020
@ph ph added v7.8.0 and removed needs_backport PR is waiting to be backported to other branches. labels Apr 7, 2020
ph pushed a commit to ph/beats that referenced this pull request Apr 7, 2020
[Agent] Inject index with sane defaults to configuration  (elastic#16903)

(cherry picked from commit 96e4506)
ph added a commit that referenced this pull request Apr 7, 2020
…7579)

[Agent] Inject index with sane defaults to configuration  (#16903)

(cherry picked from commit 96e4506)

Co-authored-by: Michal Pristas <michal.pristas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Agent] Provide sane defaults for index name [Agent] Follow new structure of configuration
4 participants