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

Metricbeat: Support wildcards in jolokia (#6453) #6462

Merged
merged 4 commits into from Mar 26, 2018

Conversation

Projects
None yet
4 participants
@jsoriano
Member

jsoriano commented Feb 23, 2018

Partially support wildcards in jolokia. This would cover the case of #6453 where wildcard is used to match one of two possible names. In a more general case this implementation is incomplete because if multiple mbeans match, their field mapping will collide and only one value will be stored.

Update: The option finally chosen and implemented is:

  • When using wildcards, each mbean generates its own event (as in option 1 below).
  • Additionally, a new event parameter is added to field mappings, to explicitly separate metrics that have different values.

To solve this I see two options:

  1. Generate an event for each matching mbean, so each event has a field with the name of the mbean. For example, current events are like this:
{
  "port": 8080,
  "max_connections": 200,
  "_namespace": "testnamespace"
}

New events would contain also the mbean, so with a wildcard these events could be generated:

{
  "port": 8080,
  "max_connections": 200,
  "mbean": "Catalina:name=\"http-bio-8080\",type=ThreadPool",
  "_namespace": "testnamespace"
}

{
  "port": 8009,
  "max_connections": 200,
  "mbean": "Catalina:name=\"ajp-bio-8009\",type=ThreadPool",
  "_namespace": "testnamespace"
}
  1. Add a regular expression or similar to the attributes mapping that can be used to fill placeholders in the field name, e.g:
jmx.mappings:
       mbean: 'Catalina:name=*,type=ThreadPool'
       attributes:
                  attr: maxConnections
                  field: "${name}.max_connections"
                  mbeanFields: 'Catalina:name=(?P<name>[^,]),type=ThreadPool'

Would generate events like this one:

{
  "http-bio-8080": {
    "port": 8080,
    "max_connections": 200
  },
  "ajp-bio-8009": {
    "port": 8009,
    "max_connections": 200
  },
  "_namespace": "testnamespace"
}

I think option 1 is more intuitive, but I'm not sure if generating multiple events fits with the general design of the module, opinions?

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Feb 26, 2018

Collaborator

Could you share an example of two json events on how they would look like with option 1?

Collaborator

ruflin commented Feb 26, 2018

Could you share an example of two json events on how they would look like with option 1?

@ruflin ruflin added the module label Feb 26, 2018

@jsoriano

This comment has been minimized.

Show comment
Hide comment
@jsoriano

jsoriano Mar 6, 2018

Member

@ruflin I have added an example for option 1 to the description.

Member

jsoriano commented Mar 6, 2018

@ruflin I have added an example for option 1 to the description.

@jsoriano

This comment has been minimized.

Show comment
Hide comment
@jsoriano

jsoriano Mar 6, 2018

Member

I guess that something like option 2 fits better with EventFetcher interface, so for a Fetch() an only event is generated.

Member

jsoriano commented Mar 6, 2018

I guess that something like option 2 fits better with EventFetcher interface, so for a Fetch() an only event is generated.

@jsoriano

This comment has been minimized.

Show comment
Hide comment
@jsoriano

jsoriano Mar 7, 2018

Member

After talking offline with @exekias about this I will go for option 1 implementing the EventsFetcher interface that allows to generate multiple events in an only call.

Member

jsoriano commented Mar 7, 2018

After talking offline with @exekias about this I will go for option 1 implementing the EventsFetcher interface that allows to generate multiple events in an only call.

@jsoriano

This comment has been minimized.

Show comment
Hide comment
@jsoriano

jsoriano Mar 8, 2018

Member

After playing a bit with Jolokia and metricbeat I think that we cannot just generate an event for each mbean as I though. I see two main use cases for this module:

  1. The module is used to obtain metrics from different mbeans in an only event, this was already supported and it makes sense that we continue supporting it. For this use case we will have metrics taken from different specific mbeans into the same event.
  2. The module is used to obtain metrics from multiple unknown sources. For that wildcards are used and each matching mbean generates a different event with its own metrics. #6453 would fit into this use case even if it only matches with one mbean and generates an only event. In this case an mbean field is added to the event so it can be used for filtering.
Member

jsoriano commented Mar 8, 2018

After playing a bit with Jolokia and metricbeat I think that we cannot just generate an event for each mbean as I though. I see two main use cases for this module:

  1. The module is used to obtain metrics from different mbeans in an only event, this was already supported and it makes sense that we continue supporting it. For this use case we will have metrics taken from different specific mbeans into the same event.
  2. The module is used to obtain metrics from multiple unknown sources. For that wildcards are used and each matching mbean generates a different event with its own metrics. #6453 would fit into this use case even if it only matches with one mbean and generates an only event. In this case an mbean field is added to the event so it can be used for filtering.
@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Mar 12, 2018

Collaborator
  • For 1 event per mbean I'm wondering on how many metrics we expect per mbean?
  • The reason I prefer multiple mbeans in one event is that the user can always configure metricbeat to have multiple instances of the metricset and configure 1 mbean per event if he wants to (except for the wildcard).
  • Should we make it a config option if it should be 1 or multiple events?
  • For the name conflicts, can saftemapstr help here?
Collaborator

ruflin commented Mar 12, 2018

  • For 1 event per mbean I'm wondering on how many metrics we expect per mbean?
  • The reason I prefer multiple mbeans in one event is that the user can always configure metricbeat to have multiple instances of the metricset and configure 1 mbean per event if he wants to (except for the wildcard).
  • Should we make it a config option if it should be 1 or multiple events?
  • For the name conflicts, can saftemapstr help here?
@jsoriano

This comment has been minimized.

Show comment
Hide comment
@jsoriano

jsoriano Mar 14, 2018

Member

@ruflin thanks for your comments, find answers inline.

  • For 1 event per mbean I'm wondering on how many metrics we expect per mbean?

It will depend on the mappings the user configures, for example for the case mentioned in #6453 (comment) it'd be about 20 metrics per matching mbean, but it could be only 2 as in the example I posted.

  • The reason I prefer multiple mbeans in one event is that the user can always configure metricbeat to have multiple instances of the metricset and configure 1 mbean per event if he wants to (except for the wildcard).

The case of building an only event from multiple known mbeans would still be supported, as well as the case of generating multiple events for known mbeans using multiple instances of the module. Multiple events would only be generated when using wildcards.

  • Should we make it a config option if it should be 1 or multiple events?

I think this would be more confusing, in general if a wildcard is used, multiple matches of similar things should be expected, so as I see it, multiple events should be generated to avoid conflicts with field names, and to support the case of #6453 (comment) that I think it fits better with the use of wildcards.
If no wildcard is used, then the user is building an only event from one or multiple known mbeans (the case supported now).
There could be a case where the user wants to build an event from mixed known mbeans and a wildcard that is expected to match only once, this seems a tricky corner case, but it could be actually supported by allowing to force the generation of an only event no matter what.
What do you think, should we support this case?

  • For the name conflicts, can saftemapstr help here?

Well, conflicts here wouldn't come from having dots in the names but by having directly multiple fields with exactly the same name coming from similar mbeans.
On the other hand, names for the fields come from user configuration, so if it has conflicts by dots it could be avoided by defining proper names, we could notice that in the documentation. Or we could use safemapstr to prevent problems with this... Opinions?

Member

jsoriano commented Mar 14, 2018

@ruflin thanks for your comments, find answers inline.

  • For 1 event per mbean I'm wondering on how many metrics we expect per mbean?

It will depend on the mappings the user configures, for example for the case mentioned in #6453 (comment) it'd be about 20 metrics per matching mbean, but it could be only 2 as in the example I posted.

  • The reason I prefer multiple mbeans in one event is that the user can always configure metricbeat to have multiple instances of the metricset and configure 1 mbean per event if he wants to (except for the wildcard).

The case of building an only event from multiple known mbeans would still be supported, as well as the case of generating multiple events for known mbeans using multiple instances of the module. Multiple events would only be generated when using wildcards.

  • Should we make it a config option if it should be 1 or multiple events?

I think this would be more confusing, in general if a wildcard is used, multiple matches of similar things should be expected, so as I see it, multiple events should be generated to avoid conflicts with field names, and to support the case of #6453 (comment) that I think it fits better with the use of wildcards.
If no wildcard is used, then the user is building an only event from one or multiple known mbeans (the case supported now).
There could be a case where the user wants to build an event from mixed known mbeans and a wildcard that is expected to match only once, this seems a tricky corner case, but it could be actually supported by allowing to force the generation of an only event no matter what.
What do you think, should we support this case?

  • For the name conflicts, can saftemapstr help here?

Well, conflicts here wouldn't come from having dots in the names but by having directly multiple fields with exactly the same name coming from similar mbeans.
On the other hand, names for the fields come from user configuration, so if it has conflicts by dots it could be avoided by defining proper names, we could notice that in the documentation. Or we could use safemapstr to prevent problems with this... Opinions?

jsoriano added some commits Feb 23, 2018

Metricbeat: Multiple events with Jolokia wildcards
When using wildcards with Jolokia, if multiple mbeans match, they will
each one have their own metrics. If we only generate an event each match
will overwrite the previous ones.

Previous behaviour is kept for Jolokia requests without wildcards, this
can be used to compose an only event from multiple specific mbeans.
Show outdated Hide outdated metricbeat/module/jolokia/jmx/config.go Outdated
Show outdated Hide outdated metricbeat/module/jolokia/jmx/config.go Outdated
@jsoriano

This comment has been minimized.

Show comment
Hide comment
@jsoriano

jsoriano Mar 22, 2018

Member

As talked offline, and in the line of #6585, I have added explicit grouping of metrics. An optional event parameter can be added to each attribute mapping so metrics with the same event are sent in the same event to Elastic.

Member

jsoriano commented Mar 22, 2018

As talked offline, and in the line of #6585, I have added explicit grouping of metrics. An optional event parameter can be added to each attribute mapping so metrics with the same event are sent in the same event to Elastic.

Metricbeat: Add option to Jolokia/JMX mappings to explicitely group m…
…etrics

A new optional `event` option is added to attribute mappings, so
attributes with the same `event` are sent in the same event to Elastic
and attributes with different `event` are sent in different events.

Some additional related refactorings done to improve type safety.
@jsoriano

This comment has been minimized.

Show comment
Hide comment
@jsoriano

jsoriano Mar 23, 2018

Member

jenkins, test it again please

Member

jsoriano commented Mar 23, 2018

jenkins, test it again please

@ruflin

ruflin approved these changes Mar 26, 2018

// Get the mapping options for the attribute of an mbean
func (m AttributeMapping) Get(mbean, attr string) (Attribute, bool) {
a, found := m[attributeMappingKey{mbean, attr}]

This comment has been minimized.

@ruflin

ruflin Mar 26, 2018

Collaborator

Nit: Seems like you could return here directly.

@ruflin

ruflin Mar 26, 2018

Collaborator

Nit: Seems like you could return here directly.

This comment has been minimized.

@jsoriano

jsoriano Mar 26, 2018

Member

I think it doesn't work when the expression is a value obtained from a map.

@jsoriano

jsoriano Mar 26, 2018

Member

I think it doesn't work when the expression is a value obtained from a map.

@ruflin ruflin merged commit 7ac8e2f into elastic:master Mar 26, 2018

6 checks passed

CLA Commit author has signed the CLA
Details
beats-ci Build finished.
Details
codecov/patch 81.48% of diff hit (target 58.49%)
Details
codecov/project 62.37% (+3.87%) compared to c472994
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!
@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Mar 26, 2018

Collaborator

@jsoriano Thanks for pushing this forward. Could you update the PR message above with the end solution you went with so people coming to this PR know directly what changed?

Would be great to have a blog post about jolokia/jmx monitoring.

Collaborator

ruflin commented Mar 26, 2018

@jsoriano Thanks for pushing this forward. Could you update the PR message above with the end solution you went with so people coming to this PR know directly what changed?

Would be great to have a blog post about jolokia/jmx monitoring.

@jsoriano

This comment has been minimized.

Show comment
Hide comment
@jsoriano

jsoriano Mar 26, 2018

Member

Updated description

Member

jsoriano commented Mar 26, 2018

Updated description

@NeQuissimus

This comment has been minimized.

Show comment
Hide comment
@NeQuissimus

NeQuissimus Mar 29, 2018

Is this going to make it into 6.3 or left for 7.0?

NeQuissimus commented Mar 29, 2018

Is this going to make it into 6.3 or left for 7.0?

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Mar 30, 2018

Collaborator

@NeQuissimus Will be in 6.3

Collaborator

ruflin commented Mar 30, 2018

@NeQuissimus Will be in 6.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment