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

461 add custom jfr events to self #505

Merged

Conversation

jan-law
Copy link
Contributor

@jan-law jan-law commented Jun 8, 2021

Related (#461)

@hareetd and I recreated this WebServerRequestEvent together by following @andrewazores' example.

(From now on, I'll be creating a separate PR for each event)

@jan-law jan-law requested a review from andrewazores June 8, 2021 15:50
@jan-law
Copy link
Contributor Author

jan-law commented Jun 8, 2021

@andrewazores Since we are not using the event fields (remoteAddr, method, path, statusCode) in the rest of our code, SpotBugs outputs a URF_UNREAD_FIELD error for all four fields. Are we missing anything that will specify that this class is an event, or should we add a @SuppressFBWarnings annotation?

@andrewazores
Copy link
Member

@andrewazores Since we are not using the event fields (remoteAddr, method, path, statusCode) in the rest of our code, SpotBugs outputs a URF_UNREAD_FIELD error for all four fields. Are we missing anything that will specify that this class is an event, or should we add a @SuppressFBWarnings annotation?

Ah, that's annoying. I think the @SuppressFBWarnings is needed here.

@andrewazores
Copy link
Member

Looking good so far.

Please read over Part II and Part III in this doc: https://docs.oracle.com/en/java/javase/14/jfapi/event-threshold.html#GUID-7EAD62F2-1769-4486-9A61-29F40D121E1F (@hareetd too)

I didn't cover it in my quick demo initially, but evt.shouldCommit() is important here. Some of the other annotations might be nice to include, ex. @Name, @Category (maybe a Cryostat category is appropriate). @StackTrace is also interesting - we probably don't care about stack traces for most of the event types we might be recording, however for webserver requests in particular it might make sense to give context for request failures. This could be a setting we specify in the template, however.

@hareetd
Copy link
Contributor

hareetd commented Jun 9, 2021

We read over the docs but we're unsure about what the threshold should be for our usage of evt.shouldCommit(). The docs say "it's recommended to set a threshold if an operation occurs frequently and outliers are of greatest concern", so we're assuming that we only care about recording webserver requests which timeout?

Based on this we've decided to set the threshold to be the timeout duration specified here:

https://github.com/cryostatio/cryostat/blob/main/src/main/java/io/cryostat/net/web/http/generic/TimeoutHandler.java#L50

However, we don't think this is right because of how rare it would be for a request to take this long, meaning such events would rarely, if ever, be recorded.

@andrewazores
Copy link
Member

I think the default threshold can simply be 0ms, ie always record this event. This can be set in templates instead - we might have different templates with different values for this setting. shouldCommit() would allow this event to be conditionally recorded depending on those different template settings, I think.

@andrewazores
Copy link
Member

@jan-law looks good to me. Are you planning to make any further changes? If not, I will merge this now.

@jan-law
Copy link
Contributor Author

jan-law commented Jun 14, 2021

@jan-law looks good to me. Are you planning to make any further changes? If not, I will merge this now.

Good to go, ready to merge

@andrewazores andrewazores merged commit 96b1647 into cryostatio:main Jun 14, 2021
@jan-law jan-law deleted the 461-add-custom-jfr-events-to-self branch June 14, 2021 17:44
andrewazores pushed a commit to andrewazores/cryostat that referenced this pull request Jun 24, 2021
* Add WebServerRequestEvent

* fixup! Add WebServerRequestEvent

* Update template desc and provider

* Supress unused event field warnings

* fixup! Supress unused event field warnings

* Add annotations

* fixup! Add annotations

* Move threshold setting to template

* Fix name typo

* Separate ip and port fields

* Rename 'ip' field to 'host'
aali309 pushed a commit to aali309/cryostat-legacy that referenced this pull request Jul 22, 2024
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