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

switch to no op monitor registry by default #466

Merged
merged 2 commits into from
Apr 13, 2020
Merged

Conversation

brharrington
Copy link
Contributor

Internally Servo is deprecated and delegates to Spectator.
This change would reduce the overhead for things that are
still using the Servo APIs. The main benefit is that the
data would no longer be exported to JMX.

Note, this could be quite disruptive to anyone else using
Servo. Since it is all setup via sytem properties, there
isn't a convenient way to only change the default we use
internally.

After making this change, I noticed that loading the JMX
monitor registry explicitly using the property no longer
worked. That has been fixed so the old behavior can be
acheived using a system property.

Internally Servo is deprecated and delegates to Spectator.
This change would reduce the overhead for things that are
still using the Servo APIs. The main benefit is that the
data would no longer be exported to JMX.

Note, this could be quite disruptive to anyone else using
Servo. Since it is all setup via sytem properties, there
isn't a convenient way to only change the default we use
internally.

After making this change, I noticed that loading the JMX
monitor registry explicitly using the property no longer
worked. That has been fixed so the old behavior can be
acheived using a system property.
@brharrington brharrington requested a review from dmuino April 10, 2020 03:32
Copy link
Contributor

@dmuino dmuino left a comment

Choose a reason for hiding this comment

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

I like this for Netflix but I think we should add a prominent note indicating the system property to add to get the old behavior, in case there are other users who rely on this.

@dmuino dmuino merged commit 0a3c21d into Netflix:master Apr 13, 2020
dmuino added a commit that referenced this pull request Apr 13, 2020
@brharrington
Copy link
Contributor Author

What did you have in mind? Just a section towards the top of the readme?

@brharrington brharrington deleted the noop branch April 13, 2020 19:20
@dmuino
Copy link
Contributor

dmuino commented Apr 13, 2020

Yes, something that would be easy to catch for someone looking at this project after something unexpected happens with the new version, like not being able to see the metrics under JMX.

brharrington added a commit to brharrington/servo that referenced this pull request Apr 16, 2020
Adds a prominent notice about no-op behavior to the readme
as discussed in Netflix#466.
dmuino pushed a commit that referenced this pull request Apr 16, 2020
Adds a prominent notice about no-op behavior to the readme
as discussed in #466.
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.

2 participants