Handle list by Mrod1598 · Pull Request #43 · open-telemetry/opentelemetry-java-contrib

Mrod1598 · 2021-06-29T18:57:19Z

Description:
Resolves #42 through the proposed solution of adding the ability to pass in multiple Object Names to better support adding additional mbeans under the same metric name.

Existing Issue(s):
#42

Testing:
Added test that defines multiple mbeans and then pulls those mbeans out using multiple object names.

Documentation:
Documented the new signature of otel.mbeans

* Add Support for multiple object names & add test for it

…y-java-contrib into handle-list

rmfitzpatrick · 2021-06-30T13:33:38Z

Provided a comment on the parent issue #42 (comment) that could effect approach. Overall this seems ok to me though depending on your thoughts.

Would you be willing to modify this test and its groovy script to utilize your thread pools mbean use case? https://github.com/open-telemetry/opentelemetry-java-contrib/blob/main/jmx-metrics/src/test/groovy/io/opentelemetry/contrib/jmxmetrics/OtlpIntegrationTests.groovy

Mrod1598 · 2021-08-17T18:49:58Z

Provided a comment on the parent issue #42 (comment) that could effect approach. Overall this seems ok to me though depending on your thoughts.

Would you be willing to modify this test and its groovy script to utilize your thread pools mbean use case? https://github.com/open-telemetry/opentelemetry-java-contrib/blob/main/jmx-metrics/src/test/groovy/io/opentelemetry/contrib/jmxmetrics/OtlpIntegrationTests.groovy

@rmfitzpatrick I've updated the test and moved it up to the newest version. I was wondering if you could approve the CI. Thanks!

rmfitzpatrick · 2021-08-17T19:31:23Z

jmx-metrics/src/test/groovy/io/opentelemetry/contrib/jmxmetrics/OtlpIntegrationTests.groovy

+        Gauge datapoints = metric.gauge
        then: 'they are of the expected size'
-        datapoints.dataPointsCount == 1
+        datapoints.dataPointsCount == 44


I'm not sure if this is related to intermediary proto changes but the number of datapoints is now more manageable. Would you mind adding more complete content validation?

It may be preferable overall to keep the existing integration tests as they were and add another for this feature.

moved to a separate test but had to modify the signature for configureContainers to allow me to pass in my own script.

Mrod1598 · 2021-09-09T19:04:04Z

Altered script and test to make metrics more predictable. It was passing locally but not on actions.

Mrod1598 · 2021-09-10T18:49:14Z

After testing it seems that the actions are failing through a lack of resources. The tests pass locally if docker is given enough resources but I get the same errors if I reduce the amount of memory given to docker down to 2GB. I was wondering if you had any ideas to help reduce parallelism so that the tests aren't using all of the resources available. @rmfitzpatrick

rmfitzpatrick · 2021-09-13T12:36:02Z

@Mrod1598, could you try restricting the worker count for the integration test action like was done previously? #41 (comment). Overall better helper logic around waiting for expected metrics is needed and I will try to take care of this in the near future, but that shouldn't block this.

I think it would also be helpful if you incorporated main changes/rebased to ensure this PR is still compatible with recent updates.

…y-java-contrib into main

Mrod1598 · 2021-09-24T13:24:20Z

@Mrod1598, could you try restricting the worker count for the integration test action like was done previously? #41 (comment). Overall better helper logic around waiting for expected metrics is needed and I will try to take care of this in the near future, but that shouldn't block this.

I think it would also be helpful if you incorporated main changes/rebased to ensure this PR is still compatible with recent updates.

Sorry, forgot to mention that I tried restricting the worker count and it doesn't seem to help locally either. any other suggestions?

Mrod1598 · 2021-10-07T13:18:24Z

@rmfitzpatrick seems the new way to handle the tests seemed to work

djaglowski · 2021-10-08T19:45:11Z

@rmfitzpatrick Is there anything that could be done to help move this forward?

jsuereth · 2021-10-11T13:56:06Z

jmx-metrics/src/integrationTest/resources/multi_obj_script.groovy

+])
+otel.instrument(helper, "cassandra.current_tasks", "Number of tasks in queue with the given task status.", "1",
+        ["stage_name":{ mbean -> mbean.name().getKeyProperty("scope")},
+            "task_status":{ mbean -> mbean.name().getKeyProperty("name")}],


Nice! This makes me wonder if there's any existing JMX metrics we should cleanup (after this PR) to better match semantic-conventions or latest metric-naming-guidance.

definitely have some ideas moving forward.

anuraaga

@rmfitzpatrick Can you take another look at this? Would be good to verify the new groovy API

anuraaga · 2021-10-12T00:15:17Z

.github/workflows/pr-build.yml

        name: Integration Tests
        with:
-          arguments: --stacktrace integrationTest
+          arguments: --stacktrace --max-workers 1 integrationTest


Can you revert this? Don't think it's needed anymore

anuraaga · 2021-10-12T00:19:39Z

jmx-metrics/src/integrationTest/java/io/opentelemetry/contrib/jmxmetrics/MultiObjTest.java

+import org.testcontainers.junit.jupiter.Container;
+import org.testcontainers.utility.MountableFile;
+
+class MultiObjTest extends AbstractIntegrationTest {


Since this looks like a new feature of the helper rather than a new backend integration I think it's better not to add yet another integration test to make sure they don't get too slow.

How about adding unit test to instrumenthelpertest and modifying the existing Cassandra test to exercise this?

here I had originally setup a modified version of the cassandra test and was told to revert it and add an additional test.

My second request to keep the single mbean integration test as is and add a new one with multiples was to preserve the datapoint validation that was being removed in favor of a simple size check that was apparently flakey.

I'm personally a fan of integration tests but can understand not want to balloon CI and dev times. If there is to be a unit test instead of the current approach I think checking the functionality of the InstrumentHelper with multi-mbean MBeanHelper would be a good way forward.

So, can this be merged? or are the changes required to be unit tests? @rmfitzpatrick

@anuraaga are you ok with the changes as is? Improving coverage could be a future item.

I'd like to have the test change made yes - it's why I left a comment :P The integration tests are already up to 5 min compared to 2 for unit tests so I don't want to continue trending up.

Added the Unit test and removed the integration test

djaglowski · 2021-10-19T17:51:58Z

@rmfitzpatrick Could this be merged?

rmfitzpatrick · 2021-10-19T21:42:50Z

@Mrod1598 thanks for updating. Would you be open to expanding the instrument helper tests a bit as mentioned earlier? https://github.com/observIQ/opentelemetry-java-contrib/pull/2

…r_test Add multiple object name instrument helper test

Mrod1598 · 2021-10-20T01:58:14Z

@Mrod1598 thanks for updating. Would you be open to expanding the instrument helper tests a bit as mentioned earlier? observIQ#2

Yes, I appreciate that

anuraaga

Thanks!

anuraaga · 2021-10-20T03:12:57Z

Was hoping to merge main for you but looks like I don't have access to update the branch

…ry-java-contrib into handle-list

Mrod1598 · 2021-10-20T12:46:06Z

Was hoping to merge main for you but looks like I don't have access to update the branch

yeah sorry about that didn't notice the issue with the merge. should be good to go now tho.

djaglowski and others added 7 commits June 22, 2021 16:16

WIP - quick attempt to allow multiple object names per otel.mbeans call

115ffe6

Add Support for multiple object names & add test for it

13c3898

Update Doc

4e35768

Implement PR feedback

6d5f8a3

Fix spelling & Remove space

5199b55

Add new otel.mbeans() signature for multiple ObjectNames (#1)

bf2689b

* Add Support for multiple object names & add test for it

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

994924a

…y-java-contrib into handle-list

Mrod1598 marked this pull request as ready for review June 29, 2021 19:21

Mrod1598 requested review from anuraaga, pmcollins, rmfitzpatrick and trask as code owners June 29, 2021 19:21

trask requested a review from a team July 6, 2021 16:52

Mrod1598 added 3 commits July 13, 2021 15:47

Update test

e6eda8a

Merge branch 'main' into handle-list

b70f855

Update Tests

e211fa8

rmfitzpatrick reviewed Aug 17, 2021

View reviewed changes

dehaansa and others added 2 commits August 26, 2021 14:10

Merge remote-tracking branch 'upstream/main' into main

3b88f46

Add new test

85c925b

Mrod1598 requested a review from rmfitzpatrick September 9, 2021 13:21

Update Script for more predictable metrics

5130860

Mrod1598 added 3 commits September 13, 2021 09:06

Merge remote-tracking branch 'origin/main' into handle-list

185ab73

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

950c444

…y-java-contrib into main

Add Max Workers

6fd19dd

Mrod1598 marked this pull request as draft September 30, 2021 14:47

Mrod1598 added 3 commits October 6, 2021 13:37

Merge branch 'main' into handle-list

cc77678

Move Test to new Java Tests

d4b4d9b

Spotless

b10a90c

Mrod1598 marked this pull request as ready for review October 7, 2021 13:15

jsuereth approved these changes Oct 11, 2021

View reviewed changes

anuraaga reviewed Oct 12, 2021

View reviewed changes

rmfitzpatrick approved these changes Oct 12, 2021

View reviewed changes

Mrod1598 added 4 commits October 13, 2021 09:59

Remove max workers

e9388b0

Move test to unit test

0f87b14

Spotless

8fcef7e

Remove old files

41923f8

Add multiple object name instrument helper test

da2c67e

qingling128 mentioned this pull request Oct 20, 2021

Cassandra metrics receiver GoogleCloudPlatform/ops-agent#220

Merged

Merge pull request #2 from rmfitzpatrick/handle_list_instrument_helpe…

8b54ba4

…r_test Add multiple object name instrument helper test

anuraaga approved these changes Oct 20, 2021

View reviewed changes

Mrod1598 added 2 commits October 20, 2021 08:43

Merge branch 'main' into handle-list

11fec61

Merge branch 'handle-list' of https://github.com/observIQ/opentelemet…

3247227

…ry-java-contrib into handle-list

Fix new instrumenthelpertests

8725964

rmfitzpatrick approved these changes Oct 20, 2021

View reviewed changes

rmfitzpatrick merged commit 7184f00 into open-telemetry:main Oct 20, 2021

Mrod1598 deleted the handle-list branch October 20, 2021 13:13

Mrod1598 mentioned this pull request Jan 10, 2022

REQUEST: New membership for @Mrod1598 open-telemetry/community#945

Closed

6 tasks

Conversation

Mrod1598 commented Jun 29, 2021

Uh oh!

rmfitzpatrick commented Jun 30, 2021

Uh oh!

Mrod1598 commented Aug 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mrod1598 commented Sep 9, 2021

Uh oh!

Mrod1598 commented Sep 10, 2021

Uh oh!

rmfitzpatrick commented Sep 13, 2021

Uh oh!

Mrod1598 commented Sep 24, 2021

Uh oh!

Mrod1598 commented Oct 7, 2021

Uh oh!

djaglowski commented Oct 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anuraaga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djaglowski commented Oct 19, 2021

Uh oh!

rmfitzpatrick commented Oct 19, 2021

Uh oh!

Mrod1598 commented Oct 20, 2021

Uh oh!

anuraaga left a comment

Choose a reason for hiding this comment

Uh oh!

anuraaga commented Oct 20, 2021

Uh oh!

Mrod1598 commented Oct 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants