Skip to content

Add multi attribute support#137

Merged
rmfitzpatrick merged 19 commits intoopen-telemetry:mainfrom
observIQ:add-multi-attribute-support
Dec 8, 2021
Merged

Add multi attribute support#137
rmfitzpatrick merged 19 commits intoopen-telemetry:mainfrom
observIQ:add-multi-attribute-support

Conversation

@Mrod1598
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

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

3 participants