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

fix: Check mxBean compatibility #1508

Merged
merged 1 commit into from
Oct 31, 2023
Merged

Conversation

asuresh4
Copy link
Contributor

Check for mxBean compatibility ahead of checking whether Allocated Memory is enabled. This is related to #1435 .

@asuresh4 asuresh4 requested review from a team as code owners October 31, 2023 18:06
@laurit
Copy link
Collaborator

laurit commented Oct 31, 2023

@asuresh4 this repository requires commits to be signed https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits you also need to add the gpg key to your github account. Also take a look at https://safjan.com/git-sign-previous-commit/

@asuresh4
Copy link
Contributor Author

Thanks @laurit , just amended the commit.

@breedx-splk breedx-splk merged commit 887d12f into signalfx:main Oct 31, 2023
31 checks passed
@asuresh4 asuresh4 deleted the mxbean-exception branch November 1, 2023 16:53
@asuresh4
Copy link
Contributor Author

asuresh4 commented Nov 1, 2023

@breedx-splk @laurit thanks for reviewing and merging this. Would it be possible to do a patch release including the fix? We have a customer waiting for this. If that is not a possibility when is the next planned release? Thanks again!

@laurit
Copy link
Collaborator

laurit commented Nov 1, 2023

@asuresh4 I think the next planned release would be on 16 nov. Patch release can be done if needed. It would help if you could provide a sentence for changelog that would describe what this fixes, or describe what failed without this fix.

@asuresh4
Copy link
Contributor Author

asuresh4 commented Nov 6, 2023

@laurit I opened #1515 for updating CHANGELOG. PTAL. It would be awesome to provide a patch release for the customer since they've wanted to install the agent and this exception has been blocking them.

@laurit
Copy link
Collaborator

laurit commented Nov 6, 2023

@asuresh4 is this fixing the same issue as described in #1435?

@asuresh4
Copy link
Contributor Author

asuresh4 commented Nov 6, 2023

@laurit Yes, the underlying issue is the same. Before this fix, we were invoking isThreadAllocatedMemoryEnabled() (the method that throws the exception) before checking compatibility. This fix invokes mxBeanTypeIsCompatible() first so that we don't run into an exception when doing the casting.

Specifically, the order in which these methods are invoked here: https://github.com/signalfx/splunk-otel-java/pull/1435/files#diff-02b1f2b3a1bbe74fb65beb4e221e386aef0aef86ccabfca6a9774430035955c2R39

laurit pushed a commit to laurit/splunk-otel-java that referenced this pull request Nov 7, 2023
laurit added a commit that referenced this pull request Nov 7, 2023
* fix: Check mxBean compatibility (#1508)

* Prepare 1.29.1 patch release

* Fix smoke test collector conf (#1503)

---------

Co-authored-by: Akash Suresh <[email protected]>
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.

3 participants