Skip to content

Conversation

lisguo
Copy link
Contributor

@lisguo lisguo commented Sep 18, 2025

Description of the issue

Currently if you try to use the "append-config" action on two configurations that have separate JVM configurations, it will fail.

Kafka Config:

{
  "metrics": {
    "namespace": "CWAgent",
    "metrics_collected": {
      "jmx": [
        {
          "endpoint": "localhost:1111",
          "kafka": {
            "measurement": [
              "kafka.request.time.avg",
            ]
          },
          "append_dimensions": {
            "ClusterName": "MyKafkaCluster"
          }
        },
        {
          "endpoint": "localhost:1111",
          "jvm": {
            "measurement": [
              "jvm.classes.loaded",
            ]
          },
          "append_dimensions": {
            "ProcessGroupName": "MyKafkaCluster"
          }
        }
      ]
    }
  }
}

Tomcat config:

{
  "metrics": {
    "namespace": "CWAgent",
    "metrics_collected": {
      "jmx": [
        {
          "endpoint": "localhost:2222",
          "tomcat": {
            "measurement": [
              "tomcat.sessions",
            ]
          },
          "append_dimensions": {
            "AppName": "MyTomcatServer"
          }
        },
        {
          "endpoint": "localhost:2222",
          "jvm": {
            "measurement": [
              "jvm.classes.loaded",
            ]
          },
          "append_dimensions": {
            "ProcessGroupName": "MyTomcatServer"
          }
        }
      ]
    }
  }
}

I expect this to merge the jvm configurations to this valid config:

{
  "metrics": {
    "namespace": "CWAgent",
    "metrics_collected": {
      "jmx": [
        {
          "endpoint": "localhost:1111",
          "kafka": {
            "measurement": [
              "kafka.request.time.avg",
            ]
          },
          "append_dimensions": {
            "ClusterName": "MyKafkaCluster"
          }
        },
        {
          "endpoint": "localhost:1111",
          "jvm": {
            "measurement": [
              "jvm.classes.loaded",
            ]
          },
          "append_dimensions": {
            "ProcessGroupName": "MyKafkaCluster"
          }
        },
        {
          "endpoint": "localhost:2222",
          "tomcat": {
            "measurement": [
              "tomcat.sessions",
            ]
          },
          "append_dimensions": {
            "AppName": "MyTomcatServer"
          }
        },
        {
          "endpoint": "localhost:2222",
          "jvm": {
            "measurement": [
              "jvm.classes.loaded",
            ]
          },
          "append_dimensions": {
            "ProcessGroupName": "MyTomcatServer"
          }
        }
      ]
    }
  }
}

Description of changes

Modify the mergeMap function in the translator to add a conditional if we see a "jmx" configuration, then merge the incoming jmx config into the list.

License

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Tests

Added unit test and manual testing. I appended 4 jmx configurations and see the otel receivers populated:

receivers:
    jmx/0:
        collection_interval: 1m0s
        endpoint: localhost:1111
        jar_path: /opt/aws/amazon-cloudwatch-agent/bin/opentelemetry-jmx-metrics.jar
        otlp:
            endpoint: 0.0.0.0:0
            timeout: 5s
        resource_attributes:
            ProcessGroupName: CinchJVM
        target_system: jvm
    jmx/1:
        collection_interval: 1m0s
        endpoint: localhost:2222
        jar_path: /opt/aws/amazon-cloudwatch-agent/bin/opentelemetry-jmx-metrics.jar
        otlp:
            endpoint: 0.0.0.0:0
            timeout: 5s
        resource_attributes:
            ClusterName: MyKafkaCluster
        target_system: kafka
    jmx/2:
        collection_interval: 1m0s
        endpoint: localhost:2222
        jar_path: /opt/aws/amazon-cloudwatch-agent/bin/opentelemetry-jmx-metrics.jar
        otlp:
            endpoint: 0.0.0.0:0
            timeout: 5s
        resource_attributes:
            ProcessGroupName: MyKafkaCluster
        target_system: jvm
    jmx/3:
        collection_interval: 1m0s
        endpoint: localhost:3333
        jar_path: /opt/aws/amazon-cloudwatch-agent/bin/opentelemetry-jmx-metrics.jar
        otlp:
            endpoint: 0.0.0.0:0
            timeout: 5s
        resource_attributes:
            AppName: MyTomcatServer
        target_system: tomcat
    jmx/4:
        collection_interval: 1m0s
        endpoint: localhost:3333
        jar_path: /opt/aws/amazon-cloudwatch-agent/bin/opentelemetry-jmx-metrics.jar
        otlp:
            endpoint: 0.0.0.0:0
            timeout: 5s
        resource_attributes:
            ProcessGroupName: MyTomcatServer
        target_system: jvm

Requirements

Before commiting your code, please do the following steps.

  1. Run make fmt and make fmt-sh
  2. Run make lint

Integration Tests

To run integration tests against this PR, add the ready for testing label.

@lisguo lisguo requested a review from a team as a code owner September 18, 2025 21:22
@lisguo lisguo changed the title [Bugfix] Fix JVM merging logic to append multiple jvm configurations [Bugfix] Fix JMX merging logic to append multiple jvm configurations Sep 18, 2025
@@ -34,6 +34,9 @@ func mergeMap(sourceMap map[string]interface{}, resultMap map[string]interface{}
for key, value := range sourceMap {
if rule, ok := mergeRuleMap[key]; ok {
rule.Merge(sourceMap, resultMap)
} else if key == "jmx" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this more general? Can we verify if we're looking at a list and then attempt to merge the list instead of specifying "jmx" specifically?

Copy link
Contributor

Choose a reason for hiding this comment

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

But then how would the append-config look when objects are used instead of an array?

Copy link
Contributor

@musa-asad musa-asad Sep 18, 2025

Choose a reason for hiding this comment

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

I think we can have two cases: 1) two arrays and 2) an array and an object. We could set up two conditions that check for each case and we can implement logic to merge an array and an object together (if we don't have that.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added an allowlist, not sure the best way to detect the case where we are merging two jmx objects and turn it into an array.

Right now I have it so that we have multiple different cases:

  1. Merging two jvm sections which are maps, if both are identical we only have a single configuration
  2. Merge two jvm sections which are maps, they are not identical -> have an array with two jvm objects
  3. Merge one jvm section that is an array with a map that are not identical -> two jvm objects in an array
  4. Merge two jvm sections which are arrays, they are not identical -> two jvm objects in array
  5. Merge two jvm sections which are arrays, they are identical -> an array with a single object

@lisguo lisguo added the ready for testing Indicates this PR is ready for integration tests to run label Sep 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for testing Indicates this PR is ready for integration tests to run
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants