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

JMX support #6131

Closed
maurolscla opened this issue Jun 1, 2022 · 23 comments
Closed

JMX support #6131

maurolscla opened this issue Jun 1, 2022 · 23 comments
Assignees
Labels
enhancement New feature or request

Comments

@maurolscla
Copy link

maurolscla commented Jun 1, 2022

Is your feature request related to a problem? Please describe.
I'd like to collect JMX metrics using a single agent rather than having to rely on another process/workload to gather JMX data

Describe the solution you'd like
Make exporting JMX metrics part of the default feature set of the OTEL Java agent

Describe alternatives you've considered
I considered running the JMX metrics gatherer project but we operate in a highly regulated/secure environment and opening up another port for scraping JMX data is not an option.

@maurolscla maurolscla added the enhancement New feature or request label Jun 1, 2022
@Samudraneel24
Copy link
Contributor

I'd like to work on this issue. Can this be assigned to me?

@trask
Copy link
Member

trask commented Jun 20, 2022

sure, assigned!

@PeterF778
Copy link
Contributor

Should we make JMX metrics collection part of the Java agent code, or provide an agent extension (to be used with the -Dotel.javaagent.extensions option)?

@trask
Copy link
Member

trask commented Jun 24, 2022

I personally think this is pretty useful and should be part of the Java agent code (e.g. we have similar functionality in our distro https://docs.microsoft.com/en-us/azure/azure-monitor/app/java-standalone-config#jmx-metrics).

Unfortunately, in otel javaagent we only have property-based configuration (yaml-based configuration is being discussed at the broader OTel level, and we will probably add that at some point which would be very helpful for other things also, e.g. rule-based sampler configuration for #1060).

So for now we may need to be creative how to configure, e.g. maybe?

otel.javaagent.jmx-metrics.1.objectName=
otel.javaagent.jmx-metrics.1.attributeNames=

It would be nice if objectName can be an objectName pattern, and attributeNames can be nested, e.g. Usage.used.

It would also be nice if there was an automatic metric naming based on objectName + attributeName, similar what @jack-berg has proposed for kafka metrics in #6138.

@PeterF778
Copy link
Contributor

Yes, yaml-based configuration would be nice! We are eager to add it in the future.
In the first version we'd like to focus on well tailored sets of predefined metrics per platform, similar to what JMX Metric Gatherer does. The configuration options will be limited.

@trask
Copy link
Member

trask commented Jun 27, 2022

In the first version we'd like to focus on well tailored sets of predefined metrics per platform, similar to what JMX Metric Gatherer does. The configuration options will be limited.

this makes sense to me 👍

@mateuszrzeszutek
Copy link
Member

So for now we may need to be creative how to configure, e.g. maybe?

otel.javaagent.jmx-metrics.1.objectName=
otel.javaagent.jmx-metrics.1.attributeNames=

Alternatively, we could add a single property that points to a yaml configuration file (or many of them, in case we want to have "standard" config files for various frameworks/systems) -- kind of how the view file configuration works.

@jack-berg
Copy link
Member

Alternatively, we could add a single property that points to a yaml configuration file (or many of them, in case we want to have "standard" config files for various frameworks/systems) -- kind of how the view file configuration works.

Don't all of these configuration options conflate responsibilities with views? With views we have a mechanism to select and disable instruments. It's already fairly powerful, allowing you to select by instrument name (with wildcard syntax), instrument type, and meter name. And the file based configuration makes it easy to configure views while using the agent without having to write any extension.

Why not just have a general flag that enables or disables all jmx metrics, powered by generic mapping rules? The readme could include a description of the metrics that are produced, and how they can be selected and disabled with views.

@PeterF778
Copy link
Contributor

I see two issues with this approach. One is that there could be literally thousands of JMX metrics available, collecting them all just to get mostly discarded would not be efficient. The second one is that there could be application specific MBeans, which we know nothing about. We could discover them, but we would not have any idea how to report them - are they monotonic? Are they providing Long or Double values? What kind of attributes(dimensions) do they need? etc.

@PeterF778
Copy link
Contributor

Until we have configuration file for JMX metrics, the following seems feasible (following @trask suggestion):

otel.javaagent.jmx-metrics.1.metricName=process.runtime.jvm.memory.usage
otel.javaagent.jmx-metrics.1.objectName='java.lang:name=*,type=MemoryPool'
otel.javaagent.jmx-metrics.1.attributeName=Usage.used
otel.javaagent.jmx-metrics.1.labels='pool=prop(name),type=attr(Type)'
otel.javaagent.jmx-metrics.1.description=
otel.javaagent.jmx-metrics.1.unit=

I'm not a big fan of appending attribute names to the metric name, I think using metric attributes (dimensions/labels) is a better way, as it allows easier data aggregation (even if it not always makes sense).

@trask
Copy link
Member

trask commented Jun 30, 2022

in the labels example, what is the proposed definitions for prop(...) and attr(...)?

what do you think of going ahead with a yaml configuration similar to what was done for metric view configuration, and having a single property that points to the yaml file?

if you had a richer configuration format (yaml), would you decompose the labels field further?

@PeterF778
Copy link
Contributor

The proposed syntax for labels would support:

  • a constant string
  • a property extracted from the corresponding ObjectName: prop(key)
  • an attribute value extracted from MBean: attr(key)
    Hopefully this would cover most needs, but it could be extended, if necessary.

@PeterF778
Copy link
Contributor

Certainly, configuring JMX metrics via system properties is not very convenient (and it looks lame), let's continue with a yaml configuration file. Here is the proposed syntax - the intention is to balance simplicity with expressiveness.

conf:
  - def:
      domain: DOMAIN             # optional
      bean:                      # optional, but the domain or at least one bean must be present
        - BEAN1                  # DOMAIN will be correctly prepended, if present
        - BEAN2                  # can contain wildcards
      tag:                       # optional, it applies to all metrics below
        - key: LABEL_KEY1
          param: PARAMETER       # used as the key to extract value from ObjectName
        - key: LABEL_KEY2
          attrib: ATTRIBUTE      # used as the attribute name to extract value from MBean
      attribute:
        ATTRIBUTE1:              # an MBean attribute name defining the metric value
          metric:
            name: METRIC_NAME1
            type: counter        # optional, the default is gauge
            desc: DESCRIPTION1   # optional
            unit: UNIT1          # optional
          tag:                   # optional, will be used in addition to the shared tags above
            - key: LABEL_KEY3
              value: CONSTANT    # direct value for the label
        ATTRIBUTE2:              # use a.b to get access into CompositeData
          metric:
            name: METRIC_NAME2
            type: gauge

@trask
Copy link
Member

trask commented Jul 6, 2022

makes sense to me 👍

do you have an example in mind where multiple bean entries are useful?

my only other thought is whether having the optional domain component is worth it, I'm guessing the easiest thing for most people is to copy the object name as a whole into the bean entry(ies)

@PeterF778
Copy link
Contributor

PeterF778 commented Jul 6, 2022

I do not have very strong arguments for either, as they are redundant, but they can sometimes make the user's life easier. For example, consider a group of MBeans which use the request URL as part of their ObjectName. The user might be interested in just a few critical URLs, providing a definition like this:

conf:
  - def:
      bean:
        - Catalina:type=Manager,host=*,context=/URL1
        - Catalina:type=Manager,host=*,context=/URL2
      attribute:
        sessionAverageAliveTime:
          metric:
            name: catalina.session.alive.time
            desc: Average session alive time
            unit: s
          tag:
            - key: host
              param: host
            - key: url
              param: context

instead of getting dozens or hundreds of not interesting values.

Singling out the domain on a separate line (which is fully optional) might make reading and maintaining the file easier, I had no other goal in mind for this.

@trask
Copy link
Member

trask commented Jul 7, 2022

For example, consider a group of MBeans which use the request URL as part of their ObjectName

makes sense, and the single MBean form can still be written in simpler form (I think?):

conf:
  - def:
      bean: BEAN1
      attribute:
        - ATTRIBUTE1
        - ATTRIBUTE2

I'd personally leave out the domain, I think it gives one more place for users to get confused vs keeping it simple and just copy pasting the object name into the bean section.

Maybe related, what do you think of renaming bean to objectName? I do like bean, but objectName might be a little more obvious what is supposed to go in the field.

On other thought, is the def node needed? The extend of my yaml knowledge is from github actions, but I think we could write:

conf:
  - bean: BEAN1
    attribute:
      - ATTRIBUTE1
      - ATTRIBUTE2
  - bean: BEAN2
    attribute:
      - ATTRIBUTE3
      - ATTRIBUTE4

@PeterF778
Copy link
Contributor

Yes, thank you for your comments. For a single MBean the simplified format should be supported, and I'm fine with removing domain and def. I do not insist on bean either, but it is shorter than objectName and avoids potential confusion with case-sensitivity. And I believe most users will look at existing definition examples before writing their own.

@trask
Copy link
Member

trask commented Jul 8, 2022

I'm good with bean 👍

@PeterF778
Copy link
Contributor

Another change suggestion: instead of specifying type as counter or gauge, which requires some reading about OTel metrics, lets just the user say type: monotonic whenever they have the respective knowledge about the metric values.

@PeterF778
Copy link
Contributor

After several iterations, trying to strike a balance between different needs, here is the latest proposal for the YAML syntax:

conf:
  - bean: OBJECTNAME1          # can contain wildcards
    beans:                     # alternatively, if multiple object names are needed
      - OBJECTNAME2            # at least one object name must be specified
      - OBJECTNAME3
    tag:                       # optional metric attributes (labels), they apply to all metrics below
      LABEL1: param(PARAM)     # PARAM is used as the key to extract value from actual ObjectName
      LABEL2: attrib(ATTRIB)   # ATTRIB is used as the attribute name to extract value from MBean
    prefix: METRIC_NAME_PREFIX # optional, useful for avoiding specifying metric names below
    mapping:
      ATTRIBUTE1:              # an MBean attribute name defining the metric value
        metric: METRIC_NAME1   # metric name will be <METRIC_NAME_PREFIX><METRIC_NAME1>
        type: monotonic        # optional
        desc: DESCRIPTION1     # optional
        unit: UNIT1            # optional
        tag:                   # optional, will be used in addition to the shared tags above
          LABEL3: STRING       # direct value for the label
      ATTRIBUTE2:              # use a.b to get access into CompositeData
        metric: METRIC_NAME2   # optional, the default is the MBean attribute name
        unit: UNIT2            # optional
      ATTRIBUTE3:              # metric name will be <METRIC_NAME_PREFIX><ATTRIBUTE3>
      ATTRIBUTE4:              # metric name will be <METRIC_NAME_PREFIX><ATTRIBUTE4>

This syntax hopefully allows users to write configurations with precise metric definitions, and also to write concise configurations if they only want to cover a lot of attributes without caring about much else.

@jack-berg
Copy link
Member

Couple of thoughts:

  • The name of the key tag isn't quite right because we're describing attributes. I know that MBeans also use the term "attribute" so the word is overloaded. May we change tag => metric_attributes to disambiguate?
  • Type should reflect the different types of instruments that are available. All of these will presumably use asynchronous (or observable) instruments. There are three options available: counter, up down counter, gauge. An async counter implies you're observing a monotonically increasing sum. A async up down counter implies you're observing a non-monotonic sum. A async gauge implies you're observing a value that can't be aggregated across dimensions with a sum. It would be better to use these instruments as the options for type, and let monotonicity be a byproduct of the instrument.

@PeterF778
Copy link
Contributor

Thanks for your comments.
Yes, "attributes" are everywhere. I chose tag because it was super short (label was my second choice). How about dimensions?
You are right about the metric type. I tried to isolate the users from the OTel metrics terminology, but it was probably a bad idea. Let's keep gauge as the default type and let users declare a different instrument type if needed.

PeterF778 added a commit to PeterF778/opentelemetry-java-instrumentation that referenced this issue Sep 9, 2022
Solving  open-telemetry#6131 (JMX Support).
PeterF778 added a commit to PeterF778/opentelemetry-java-instrumentation that referenced this issue Sep 22, 2022
Solving  open-telemetry#6131 (JMX Support).
trask added a commit that referenced this issue Nov 16, 2022
Solving
#6131
(JMX Support).

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
@PeterF778
Copy link
Contributor

This issue can be closed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants