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.

Sign up for GitHub

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

Jump to bottom

Add getters for AlertEvent #5522

Merged
merged 5 commits into from
Apr 12, 2018
Merged

Add getters for AlertEvent #5522

merged 5 commits into from
Apr 12, 2018

Conversation

a2l007
Copy link
Contributor

@a2l007 a2l007 commented Mar 22, 2018

Based out of #5505
This PR adds getter methods for specific fields of AlertEvent which were removed previously. Although these getter methods did not have any usages in any of the community extensions, they were being used in our internal emitter extensions.
The AlertEvent dimensions could be useful for other emitter extensions as well and could be a @publicapi in the future.

@leventov
Copy link
Member

Please add appropriate @PublicApi annotations in this PR. Also, the restored methods should be annotated @SuppressWarnings("unused") with comments that they are used in some proprietary extensions.

@a2l007
Copy link
Contributor Author

a2l007 commented Mar 22, 2018 •
edited
Loading

@leventov @PublicApi and other druid specific annotations are not accessible from java-util. Would it make sense to move the emitter related code over to druid-api?

@leventov
Copy link
Member

It's going to happen in #5490

@a2l007
Copy link
Contributor Author

a2l007 commented Mar 23, 2018

Will make the changes once that PR is merged.

@a2l007
Copy link
Contributor Author

a2l007 commented Apr 9, 2018

@leventov Looks like #5490 isn't happening. Any suggestions on how to proceed with this PR?

@leventov
Copy link
Member

leventov commented Apr 9, 2018

Yes. I suggest you to move @PublicApi and @ExtensionPoint up to java-util module, without changing it's package though.

@a2l007
Copy link
Contributor Author

a2l007 commented Apr 9, 2018

@leventov I have made the requested changes. Please review.

leventov
leventov reviewed Apr 9, 2018
View reviewed changes
java-util/src/main/java/io/druid/java/util/emitter/service/AlertEvent.java Outdated
/*
* This method is used in certain proprietary emitter extensions
*/
@PublicApi
Copy link
Member

Choose a reason for hiding this comment

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

@PublicApi should be on the class itself, not individual methods

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@leventov
Copy link
Member

leventov commented Apr 9, 2018

In #5505 @gianm mentioned other classed to be annotated

@a2l007
Copy link
Contributor Author

a2l007 commented Apr 9, 2018

@gianm I've made ServiceMetricEvent and RequestLogEvent @PublicApi as you had mentioned in #5505. Could you please review?

gianm
gianm approved these changes Apr 12, 2018
View reviewed changes
Copy link
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

LGTM

@a2l007
Copy link
Contributor Author

a2l007 commented Apr 12, 2018

@leventov Do you have any additional comments?

@leventov leventov merged commit 19f3599 into apache:master Apr 12, 2018
sathishsri88 pushed a commit to sathishs/druid that referenced this pull request May 8, 2018
@a2l007
Add getters for AlertEvent (apache#5522)
7b7cc4a 
* Add getters for AlertEvent

* Move PublicApi and ExtensionPoint to java-util

* Fix publicapi annotation usage

* Add publicapi annotations to ServiceMetricEvent and RequestLogEvent
@dclim dclim added this to the 0.13.0 milestone Oct 8, 2018
@leventov leventov mentioned this pull request Oct 16, 2018
Merged
leventov added a commit that referenced this pull request Oct 31, 2018
@leventov
Add RequestLogEventBuilderFactory (#6477)
2cdce2e 
This PR allows to control the fields in `RequestLogEvent`, emitted in `EmittingRequestLogger`. In our case, we want to get rid of the `intervals` fields of the query objects that are a part of `DefaultRequestLogEvent`. They are enormous (thousands of segments) and not useful.

Related to #5522, FYI @a2l007.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leventov leventov leventov left review comments

@gianm gianm gianm approved these changes

Assignees
No one assigned
Labels
Area - Metrics/Event Emitting Design Review
Projects
None yet
Milestone
0.13.0
Development

Successfully merging this pull request may close these issues.
4 participants
@a2l007 @leventov @gianm @dclim