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

7036: Invoke converters/stringify for event fields and return values #174

Closed
wants to merge 2 commits into from

Conversation

@gunnarmorling
Copy link
Contributor

@gunnarmorling gunnarmorling commented Dec 4, 2020

@thegreystone, ran into this left-over from the converter change: no converters/stringify was invoked for field attributes. I've attributed it to the same issue you worked on.


Progress

  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JMC-7036: Invoke converters/stringify for event fields and return values

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jmc pull/174/head:pull/174
$ git checkout pull/174

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 4, 2020

👋 Welcome back gunnarmorling! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Dec 4, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 4, 2020

Webrevs

@thegreystone
Copy link
Member

@thegreystone thegreystone commented Dec 5, 2020

Will add a separate issue.

@thegreystone
Copy link
Member

@thegreystone thegreystone commented Dec 5, 2020

Ah. Should also do it for return values. Want to do that in the same issue?

@thegreystone thegreystone changed the title 6996: Invoking converters/stringify for event fields 7036: Invoke converters/stringify for event fields and return values Dec 5, 2020
@gunnarmorling
Copy link
Contributor Author

@gunnarmorling gunnarmorling commented Dec 5, 2020

Ah. Should also do it for return values.

Weird, I had checked for that but concluded it was handled already. Seems it was too late last night :)

Want to do that in the same issue?

Let's use the same, just pushed a commit for it (will update the messages once we have an issue for it).

Taking a step back, this really makes the lack of testing apparent. Would be pretty easy with JfrUnit ;)

@thegreystone
Copy link
Member

@thegreystone thegreystone commented Dec 5, 2020

It should at least test that the instrumentation doesn't generate invalid bytecode. Please add a few probes (and test methods), verifying that converters added to fields and return values doesn't break. You can check my converter commit to see how.

@thegreystone
Copy link
Member

@thegreystone thegreystone commented Dec 14, 2020

Hi @gunnarmorling - if you're low on time, I'd be happy to fix this.

@gunnarmorling
Copy link
Contributor Author

@gunnarmorling gunnarmorling commented Dec 14, 2020

Sorry for the delay, and thanks a lot for the offer. It's on my plan for this week to wrap it up.

@gunnarmorling
Copy link
Contributor Author

@gunnarmorling gunnarmorling commented Jan 4, 2021

You can check my converter commit to see how.

@thegreystone Which commit do you mean exactly? Can you share its SHA? Or are you working on this already? Sorry for the delay btw. :(

@thegreystone
Copy link
Member

@thegreystone thegreystone commented Jan 4, 2021

I can take it. NP! :)

@gunnarmorling
Copy link
Contributor Author

@gunnarmorling gunnarmorling commented Jan 4, 2021

Ok, thanks. Appreciating it!

@thegreystone
Copy link
Member

@thegreystone thegreystone commented Jan 5, 2021

Will take this as is, and fix it as part of 7008.

@openjdk
Copy link

@openjdk openjdk bot commented Jan 5, 2021

@gunnarmorling This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

7036: Invoke converters/stringify for event fields and return values

Reviewed-by: hirt

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 13 new commits pushed to the master branch:

  • eeddd1e: 7073: Exceptions in FlameGraph
  • 44d7dc8: 7078: Update the p2-maven-plugin to 1.5.0
  • a6d85ef: 7062: Update to the Eclipse 2020-12 platform
  • 2ede603: 7000: Update third party library used in flightrecorder.flameview
  • 46a5590: 7052: The link to BellSoft LMC should be updated
  • 19793da: 7055: Flameview manifest is exporting non-existent package
  • 4b73d04: 7057: Update Copyright year in About dialog and update site landing page
  • 1ec1c33: 6986: "Flight Recorder" is not working as expected for localhost Connection
  • d3a3000: 7016: JAWS only read “link” on letter small A and big A for welcome page
  • f99d387: 6840: [Accessibility, JAWS] 'Start Using JMC' link in Welcome Page is not traversable using Keyboard
  • ... and 3 more: https://git.openjdk.java.net/jmc/compare/65187e9448cf9b8ed9b629295965fd8db63262f6...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@thegreystone) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Jan 5, 2021
@gunnarmorling
Copy link
Contributor Author

@gunnarmorling gunnarmorling commented Jan 6, 2021

/integrate

@thegreystone to be sure, you're going to push another commit to wrap this one up, correct?

@openjdk openjdk bot added the sponsor label Jan 6, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 6, 2021

@gunnarmorling
Your change (at version 6b47559) is now ready to be sponsored by a Committer.

@thegreystone
Copy link
Member

@thegreystone thegreystone commented Jan 6, 2021

/sponsor

@openjdk openjdk bot closed this Jan 6, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 6, 2021

@thegreystone @gunnarmorling Since your change was applied there have been 14 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit 88731ed.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants