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

Introduce datasources in package to configure inputs and streams #212

Merged
merged 4 commits into from
Feb 18, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
type: log

input: log
paths:
{{#each paths}}
paths: "{{this}}"
- "{{this}}"
{{/each}}
ingest_pipeline: {{pipeline}}

exclude_files: [".gz$"]

processors:
Expand Down
32 changes: 20 additions & 12 deletions dev/package-examples/nginx-1.2.0/dataset/access/manifest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,26 @@ type: logs
# The ingest pipeline which should be used.
ingest_pipeline: default

vars:
- name: paths
# Should we define this as array? How will the UI best make sense of it?
type: textarea
default:
- /var/log/nginx/access.log*
# I suggest to use ECS fields for this config options here: https://github.com/elastic/ecs/blob/master/schemas/os.yml
# This would need to be based on a predefined definition on what can be filtered on
os.darwin:
- /usr/local/var/log/nginx/access.log*
os.windows:
- c:/programdata/nginx/logs/*access.log*
# List of supported inputs
inputs:
- type: log
Copy link
Contributor

Choose a reason for hiding this comment

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

@hbharding suggested today that it would be great to have descriptions in the UI. Could we add description prop for every input, under its type? I think these descriptions should only be a sentence or two, max. Not sure how we will localize these though..

Copy link
Member Author

Choose a reason for hiding this comment

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

+1. Added it. But did not add it here but on the package level where the overall inputs are defined. As a single input is shown for multiple datasets, I guess it makes more sense there.

vars:
- name: paths
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to my previous comment, it would be great to have description prop per var too. This would be really short descriptions that give the user info about what each var is used for.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.

required: true
# Should we define this as array? How will the UI best make sense of it?
description: Paths to the nginx access log file.
type: text
multi: true
default:
- /var/log/nginx/access.log*
# I suggest to use ECS fields for this config options here: https://github.com/elastic/ecs/blob/master/schemas/os.yml
# This would need to be based on a predefined definition on what can be filtered on
os.darwin:
default:
- /usr/local/var/log/nginx/access.log*
os.windows:
default:
- c:/programdata/nginx/logs/*access.log*

requirements:
elasticsearch.processors:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@

# The selected input has to be passed to the stream config when processed.

{{#if input == log}}
input: log

{{#each paths}}
paths: "{{this}}"
{{/each}}
exclude_files: [".gz$"]

processors:
- add_locale: ~
{{/if}}


# This is an example stream config on how multiple inputs could be supported

{{#if input == syslog}}
input: syslog

# TODO: would need some more config options
{{/if}}
34 changes: 26 additions & 8 deletions dev/package-examples/nginx-1.2.0/dataset/error/manifest.yml
Original file line number Diff line number Diff line change
@@ -1,11 +1,29 @@
title: Nginx Error Logs
type: logs
ingest_pipeline: pipeline
vars:
- name: paths
default:
- /var/log/nginx/error.log*
os.darwin:
- /usr/local/var/log/nginx/error.log*
os.windows:
- c:/programdata/nginx/logs/error.log*


# This is an example that multiple inputs are supported by one dataset
inputs:
- type: log
vars:
- name: paths
required: true
default:
- /var/log/nginx/error.log*

# TODO: The exact definition of os specific paths still needs to be defined
os.darwin:
- /usr/local/var/log/nginx/error.log*
os.windows:
- c:/programdata/nginx/logs/error.log*

- type: syslog
vars:
# Are udp and tcp syslog input two different inputs?
- name: protocol.udp.host
required: true
default:
- "localhost:9000"


Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
type: metric/nginx
# Defines which input to use, should be stripped out when creating the config
input: metrics/nginx
metricsets: ["stubstatus"]
period: {{period}}
enabled: true
Expand Down
21 changes: 7 additions & 14 deletions dev/package-examples/nginx-1.2.0/dataset/stubstatus/manifest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,10 @@ compatibility: linux, freebsd
# Each input can be in its own release status
release: beta

vars:
- name: hosts
description: Nginx hosts
default:
["http://127.0.0.1"]
required: true
- name: period
description: "Collection period. Valid values: 10s, 5m, 2h"
default: "10s"
- name: username
type: text
- name: password
# This is the html input type?
type: password
# List of inputs this dataset supports
inputs:
- type: "metric/nginx"

# Only the variables have to be repeated that are not specified as part of the input
vars:
# All variables are specified in the input already
47 changes: 47 additions & 0 deletions dev/package-examples/nginx-1.2.0/manifest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,50 @@ requirement:
versions: ">7.1.0 <7.6.0"
elasticsearch:
versions: ">7.0.1"

datasources:
-
# Do we need a name for the data source?
name: nginx
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 name would be useful to populate the id in agent config, but we can just use the package name too

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean prefixing the id? Because the id needs to be unique.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, I meant prefixing


# List of inputs this datasource supports
inputs:
-
# An id can be given, in case the type used here is not unique
# This is for selection in the stream
# id: nginx
type: nginx/metrics
descrition: Collecting metrics for nginx.

# Common configuration options for this input
vars:
- name: hosts
description: Nginx hosts
default:
["http://127.0.0.1"]
# All the config options that are required should be shown in the UI
required: true
Comment on lines +32 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

I wold like to see type defined explicitly for all vars. And if we like the multi suggestion in my previous comment, these should be added:

type: text
multi: true

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree. Adding.

multi: true
type: text
- name: period
description: "Collection period. Valid values: 10s, 5m, 2h"
default: "10s"
Comment on lines +40 to +42
Copy link
Contributor

Choose a reason for hiding this comment

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

Add type: duration or type: text?

Copy link

Choose a reason for hiding this comment

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

+1 for a duration type, we used integer priviously and it was not that great. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I like duration if that works for the UI.

type: duration
- name: username
type: text
- name: password
# This is the html input type?
type: password
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 we need to decide all possible values for type field. They do not need to be HTML types, though. Here are the possible types I currently see (or can foresee being useful):

text -> maps to UI text input
number -> maps to UI number input
boolean -> maps to UI checkbox
duration -> maps to UI text input
  this would let us do client-side validation but maybe overkill for now?
url -> maps to UI text input
  this would let us do client-side validation but maybe overkill for now?

In the case of password vars, I would use text for them as they'll show up as simple text inputs in the UI.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the above proposed types. I was thinking of password instead of text to not have to show clear text passwords directly on the screen when users put them in.

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW: If we have these predefined values for type we can also validate it already on the package side to make sure nothing odd shows up in the package itself.

Copy link

Choose a reason for hiding this comment

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

For the URL types we have to be careful when we do this and we do it, in some modules we do use compound scheme (not sure if this is the exact term?) Like "http+npipe://./pipe/custom" see https://github.com/elastic/beats/blob/d75261469b5c2e675d7d331b86bfc8599d797ddc/metricbeat/mb/parse/url_test.go for a few examples.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see url support as a stretch goal. In case of doubt we can always fall back to just string. Lets keep it simple for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can add granular text types (duration, url, etc) but it doesn't mean that we need to do validation right now. Validation can be added later on from all fronts (UI, agent, package) once we align on rules.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also ++ on keeping password type. This give the UI information to decide when to show clear text inputs and when to obfuscate.

Copy link
Member Author

Choose a reason for hiding this comment

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

As soon as we started the UI implementation and are happy with the format, we should open a PR here with the exact specs.


-
type: logs
description: Collect nginx logs.

# Common configuration options for this input
vars:

-
type: syslog

# Common configuration options for this input
vars:

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
input: metrics/system
enabled: false # default true
metricset: cpu
period: 10s
dataset: system.cpu
metrics: ["percentages", "normalized_percentages"]
3 changes: 2 additions & 1 deletion dev/package-examples/system-0.9.0/dataset/cpu/manifest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@ release: beta
# Needs to describe the type of this input
type: metrics


inputs:
- type: metrics/system

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
inputy: metric/system
enabled: true
metricsets:
- load
3 changes: 2 additions & 1 deletion dev/package-examples/system-0.9.0/dataset/load/manifest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@ release: beta
# Needs to describe the type of this input
type: metrics


inputs:
- type: system/metrics

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
input: metric/system
enabled: true
metricsets:
- memory
2 changes: 2 additions & 0 deletions dev/package-examples/system-0.9.0/dataset/memory/manifest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,6 @@ release: beta
# Needs to describe the type of this input
type: metrics

inputs:
- type: system/metrics

7 changes: 7 additions & 0 deletions dev/package-examples/system-0.9.0/manifest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,10 @@ release: ga
requirement:
kibana:
versions: "<8.0.0"


datasources:
- inputs:
type: metrics/system
- inputs:
type: log