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

Refactor jolokia fields #14086

Closed
wants to merge 1 commit into from

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Oct 16, 2019

This is a draft PR that possibly could resolve_ #13316

Signed-off-by: chrismark <chrismarkou92@gmail.com>
@ChrsMark ChrsMark self-assigned this Oct 16, 2019
@ChrsMark ChrsMark added Team:Integrations Label for the Integrations team Metricbeat Metricbeat refactoring labels Oct 16, 2019
@andresrc andresrc added :integrations Team:Platforms Label for the Integrations - Platforms team and removed Team:Integrations Label for the Integrations team labels Mar 9, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations-platforms (Team:Platforms)

@exekias
Copy link
Contributor

exekias commented Jun 30, 2020

This would be a breaking change so we would need to put it behind a flag, what do you think about it @jsoriano ?

@jsoriano jsoriano self-requested a review June 30, 2020 13:54
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

@ChrsMark thanks for starting to work on this! The initial approach looks good, but I think we need to do some more things to have a complete support.

As said by @exekias, we would need to put this feature behind a flag, both modes are going to need to coexist for a time, maybe forever (namespacing and explicit mapping is useful in light modules, think on the light metricsets of the kafka module, or the tomcat one). Having a flag will also allow to merge this earlier, and incrementally improve the support on this new mode. In some major version we can then decide to use the new mode by default.

fieldKey = fmt.Sprintf("jvm.jmx.metrics.string.%v", detotKey)
default:
logp.Debug("jolokia", "JMX value should be doulbe or string and received %v", v)
}
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to support all types Jolokia knows how to serialize, and we will have to check to what types they are converted when Unmarshalling. We will need a good set of test cases for this.
You can find the list of supported types here: https://jolokia.org/reference/html/protocol.html#serialization-response

Take also a look to the list operation, that allows to obtain information about the MBean attributes, including its type and description. Knowing the type we could do a proper handling without depending on how the value is converted to JSON and Unmarshalled. (With the description we could also provide some tool to autogenerate the documentation on light modules, but this is another story).

@mergify
Copy link
Contributor

mergify bot commented Sep 22, 2021

This pull request does not have a backport label. Could you fix it @ChrsMark? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Sep 22, 2021
@mergify
Copy link
Contributor

mergify bot commented Nov 10, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b jolokia_fields_refactor upstream/jolokia_fields_refactor
git merge upstream/master
git push upstream jolokia_fields_refactor

@jlind23
Copy link
Collaborator

jlind23 commented Mar 31, 2022

@ChrsMark - Closing this one as there were no activity for a while

@jlind23 jlind23 closed this Mar 31, 2022
@zube zube bot removed the [zube]: Done label Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip Skip notification from the automated backport with mergify Metricbeat Metricbeat refactoring Team:Platforms Label for the Integrations - Platforms team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants