Skip to content

Add getters for AlertEvent - #5522

Merged
leventov merged 5 commits into
apache:masterfrom
a2l007:add_getters
Apr 12, 2018
Merged

Add getters for AlertEvent#5522
leventov merged 5 commits into
apache:masterfrom
a2l007:add_getters

Conversation

@a2l007

@a2l007 a2l007 commented Mar 22, 2018

Copy link
Copy Markdown
Contributor

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

leventov commented Mar 22, 2018

Copy link
Copy Markdown
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

a2l007 commented Mar 22, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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

leventov commented Mar 23, 2018

Copy link
Copy Markdown
Member

It's going to happen in #5490

@a2l007

a2l007 commented Mar 23, 2018

Copy link
Copy Markdown
Contributor Author

Will make the changes once that PR is merged.

@a2l007

a2l007 commented Apr 9, 2018

Copy link
Copy Markdown
Contributor Author

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

@leventov

leventov commented Apr 9, 2018

Copy link
Copy Markdown
Member

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

@a2l007

a2l007 commented Apr 9, 2018

Copy link
Copy Markdown
Contributor Author

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

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

@leventov leventov Apr 9, 2018

Copy link
Copy Markdown
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

@a2l007 a2l007 Apr 9, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@leventov

leventov commented Apr 9, 2018

Copy link
Copy Markdown
Member

In #5505 @gianm mentioned other classed to be annotated

@a2l007

a2l007 commented Apr 9, 2018

Copy link
Copy Markdown
Contributor Author

@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

@gianm gianm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@a2l007

a2l007 commented Apr 12, 2018

Copy link
Copy Markdown
Contributor Author

@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 sathishs
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.
riovic918data pushed a commit to riovic918data/druid that referenced this pull request Jun 12, 2026
@a2l007 @leventov
Add getters for AlertEvent (apache#5522)
3ace9c5 
* Add getters for AlertEvent

* Move PublicApi and ExtensionPoint to java-util

* Fix publicapi annotation usage

* Add publicapi annotations to ServiceMetricEvent and RequestLogEvent
riovic918data pushed a commit to riovic918data/druid that referenced this pull request Jun 12, 2026
@leventov
Add RequestLogEventBuilderFactory (apache#6477)
1533014 
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 apache#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