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

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

Merged
merged 4 commits into from
Mar 26, 2018

Conversation

jsoriano
Copy link
Member

@jsoriano 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?

@jsoriano jsoriano added enhancement in progress Pull request is currently in progress. review Metricbeat Metricbeat discuss Issue needs further discussion. labels Feb 23, 2018
@ruflin
Copy link
Member

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

jsoriano commented Mar 6, 2018

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

@jsoriano
Copy link
Member Author

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

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.


"github.com/joeshaw/multierror"
"github.com/pkg/errors"

"github.com/elastic/beats/libbeat/common"
)

const (
MBeanEventKey = "mbean"

Choose a reason for hiding this comment

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

exported const MBeanEventKey should have comment (or a comment on this block) or be unexported

@jsoriano
Copy link
Member Author

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. Support for wildcard in jolokia module in metricbeat #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
Copy link
Member

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

@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?

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.
}
type AttributeMapping map[attributeMappingKey]Attribute

func (m AttributeMapping) Get(mbean, attr string) (Attribute, bool) {

Choose a reason for hiding this comment

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

exported method AttributeMapping.Get should have comment or be unexported

type attributeMappingKey struct {
mbean, attr string
}
type AttributeMapping map[attributeMappingKey]Attribute

Choose a reason for hiding this comment

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

exported type AttributeMapping should have comment or be unexported

@jsoriano
Copy link
Member Author

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.

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

jenkins, test it again please

@jsoriano jsoriano removed discuss Issue needs further discussion. in progress Pull request is currently in progress. labels Mar 23, 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}]
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Seems like you could return here directly.

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

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

Updated description

@NeQuissimus
Copy link

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

@ruflin
Copy link
Member

ruflin commented Mar 30, 2018

@NeQuissimus Will be in 6.3

@jsoriano jsoriano added the needs_backport PR is waiting to be backported to other branches. label Apr 3, 2018
@jsoriano jsoriano removed the needs_backport PR is waiting to be backported to other branches. label Apr 3, 2018
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.

4 participants