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

Add multi attribute support #137

Merged
merged 19 commits into from
Dec 8, 2021
Merged

Add multi attribute support #137

merged 19 commits into from
Dec 8, 2021

Conversation

Mrod1598
Copy link
Contributor

Description:
Adds support for multiple attributes through expanding on the otel.instrument signatures.

Existing Issue(s):
#136

Testing:
Added to InstrumentHelperTest

@Mrod1598 Mrod1598 requested a review from a team November 23, 2021 21:27
@mateuszrzeszutek mateuszrzeszutek linked an issue Nov 29, 2021 that may be closed by this pull request
@rmfitzpatrick
Copy link
Contributor

@Mrod1598 thanks for this. I think the implementation seems ok so it would be helpful to update the docstrings and readme to advertise the new signature(s) along with the changes.

@Mrod1598
Copy link
Contributor Author

Mrod1598 commented Dec 6, 2021

@Mrod1598 thanks for this. I think the implementation seems ok so it would be helpful to update the docstrings and readme to advertise the new signature(s) along with the changes.

I'm wording if it's redundant to add all new signatures for all of the signatures added. Do you think it's fine to add a note about here that says that attributes can be swapped out with the Map<String,Map<String,Closure>>?

@rmfitzpatrick
Copy link
Contributor

rmfitzpatrick commented Dec 6, 2021

I'm wording if it's redundant to add all new signatures for all of the signatures added. Do you think it's fine to add a note about here that says that attributes can be swapped out with the Map<String,Map<String,Closure>>?

I think it'd preferable to follow the existing pattern of the showing the instrument signature variations and introduce the new mbean attribute label function map with an example like is done with the labelFuncs. I think the whole thing could be done in 5 lines new or so.

In cases where you'd like to share instrument names while creating datapoints for multiple MBean attributes...

- `otel.instrument(MBeanHelper mBeanHelper, String instrumentName, String description, String unit, Map<String, Closure> labelFuncs, Map<String, Map<String, Closure>> attributeLabelFuncs, Closure instrument)`

<... description and example of attributeLabelFuncs with intended purpose... >

`otel.instrument()` provides additional signatures to allow this more expressive MBean attribute access:
- `otel.instrument(MBeanHelper mBeanHelper, String name, String description, String unit, Map<String, Map<String, Closure>> attributeLabelFuncs, Closure instrument)` - `labelFuncs` are empty map.
- `otel.instrument(MBeanHelper mBeanHelper, String name, String description, Map<String, Map<String, Closure>> attributeLabelFuncs, Closure instrument)` - `unit` is "1" and `labelFuncs` are empty map.
- `otel.instrument(MBeanHelper mBeanHelper, String name, Map<String, Map<String, Closure>> attributeLabelFuncs, Closure instrument)` - `description` is empty string, `unit` is "1" and `labelFuncs` are empty map

jmx-metrics/README.md Outdated Show resolved Hide resolved
Mrod1598 and others added 2 commits December 8, 2021 13:08
Co-authored-by: Ryan Fitzpatrick <rmfitzpatrick@users.noreply.github.com>
Co-authored-by: Ryan Fitzpatrick <rmfitzpatrick@users.noreply.github.com>
@rmfitzpatrick rmfitzpatrick merged commit 632081c into open-telemetry:main Dec 8, 2021
@Mrod1598 Mrod1598 deleted the add-multi-attribute-support branch December 8, 2021 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Support for Multiple Attributes
2 participants