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

Server changes for Forwarder #5670

Merged
merged 17 commits into from Mar 13, 2019

Conversation

@danotorrey
Copy link
Contributor

commented Feb 11, 2019

The Graylog Server will need several changes in order for the Graylog Forwarder to be implemented.

  1. The Graylog Forwarder feature uses the same Journaling capability that exists in the Graylog Server currently. The KafkaJournal class has been reworked to allow support for instance-based metric names.

  2. Output implementations do not currently have access to the output id. A change has been made that allows the output implementation to access the output instance and therefore the id.

See Graylog2/graylog-plugin-enterprise-integrations#30 for the core Forwarder PR.

@danotorrey danotorrey self-assigned this Feb 11, 2019

@danotorrey danotorrey changed the title Use unique metric names within KafkaJournal Use unique journal metric names for Graylog Fowarder Feb 11, 2019

@danotorrey danotorrey changed the title Use unique journal metric names for Graylog Fowarder Server changes for Forwarder Feb 13, 2019

@danotorrey danotorrey force-pushed the forwarder branch from a18ac5d to 64cccbb Feb 28, 2019

@danotorrey

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

Note that #5738 also contains changes needed for the Forwarder to work. The forwarder branch has temporarily been rebased off of graceful-shutdown-service (to allow dev/testing of the Forwarder), which we can undo before this PR is ready for merging.

@Graylog2 Graylog2 deleted a comment from bernd Feb 28, 2019

@danotorrey danotorrey force-pushed the forwarder branch 2 times, most recently from f29dbf2 to 788ecde Mar 1, 2019

@danotorrey danotorrey requested a review from bernd Mar 2, 2019

@danotorrey

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

@bernd These Forwarder server changes are ready for review.

@bernd bernd assigned bernd and danotorrey and unassigned danotorrey Mar 4, 2019

@danotorrey

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

@bernd I had to add handling to teardown KafkaJournal metrics (see f75f1c8) when the journal is shutdown to ensure that restarts of the Forwarder output are successful and don't result in a duplicate metric error (when editing and saving the output, for example). This PR is ready for review.

@@ -44,35 +49,56 @@ public MessageOutput fromStreamOutput(Output output, final Stream stream, Config
final String outputType = output.getType();
Preconditions.checkArgument(outputType != null);

// We have two output lists for backwards compatibility.
// See comments in MessageOutput.Factory and MessageOutput.Factory2 for details
final MessageOutput.Factory2<? extends MessageOutput> factory2 = this.availableOutputs2.get(outputType);

This comment has been minimized.

Copy link
@danotorrey

danotorrey Mar 5, 2019

Author Contributor

@bernd Minor question: Is there any reason why Factory2 is before Factory here? Just wondering if we want Factory to come first for consistency. Same question for the if-else block that follows.

This comment has been minimized.

Copy link
@bernd

bernd Mar 6, 2019

Member

@danotorrey The idea was that plugins using the newer factory interface should be preferred if the same plugin is registered twice for some reason. I will add a comment that explains it.

@danotorrey

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

@bernd the MessageOutput.Factory2 changes LGTM (except for one minor question above). Can you please verify that the constructor changes in Graylog2/graylog-plugin-enterprise-integrations@9add4a5 are valid?

Also wondering: What is the long-term plan for Factory2? Will we keep it forever, or is there a path to consolidate in a future major version.

@danotorrey

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

@bernd All metrics in KafkaJournal are now torn down properly on restart. I left the initialization in two steps in the constructor, since I think this was deliberate in original implementation (possibly some metrics used while constructor is executing).

@bernd

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

@danotorrey

@bernd the MessageOutput.Factory2 changes LGTM (except for one minor question above). Can you please verify that the constructor changes in Graylog2/graylog-plugin-enterprise-integrations@9add4a5 are valid?

Looks good and works for me. 👍

Also wondering: What is the long-term plan for Factory2? Will we keep it forever, or is there a path to consolidate in a future major version.

I guess we will have it until we refactor the MessageOutput. This will be a breaking change for all output plugins, so I guess it will take some time. So yeah, we will probably keep it for now.

danotorrey and others added 11 commits Feb 11, 2019
Use class-based unique metric names
Allows a subclass instance to run without throwing a "metric already exists" exceptions. This will need to be refactored later to support multiple instances. Skipping this for now to prevent CI errors in enterprise integrations repository.
Add additional MessageOutputFactory constructor including output
This allows us to access the output ID in a backwards compatible way (does not break existing 3rd party outputs)
Make KafkaJournal metrics unique across instances
The KafkaJournal had several hard-coded metrics directly referencing the KafkaJournal.class class name. This have been changed to allow a metricPrefix override, which can allow instances to provide unique metric names for each instance.
Add hack for GlobalMetricName backwards compatibility
Also revert unintended formatting changes.
Teardown metrics in shutdown method
Prevents duplicate metric error message when restarting the Forwarder (via Outputs page > Edit > Save)
Add MessageOutput.Factory2 interface for guice
New outputs need to be able to receive the Output instance via assisted
inject. We cannot just change the existing MessageOutput.Factory
interface because that would break older output plugins.

Using a "default" method for the new "create()" method also doesn't work
because guice cannot handle that and old plugins would fail to load.

The only way seems to be the introduction of a second Factory interface
that can be used by newer outputs to get the Output instance.

This change also removes "MessageOutputFactory#get(String)" because it
doesn't seem to be used anymore and it cannot work with two different
output factories.
bernd and others added 5 commits Mar 5, 2019
Teardown all metrics on shutdown
Several metrics were not being torn down at shutdown time. These have been cleaned up. The setup of metrics was left in two blocks (did not want to consolidate and potentially cause a situation where a metric is not created yet when used in the constructor)
Add support for disabling KafkaJournal throttling
According to a comment in graylog.conf, the original intent of the KafkaJournal.throttleThresholdPercentage field was to disable throttling updates if the value is not specified. This commit changes KafkaJournal to not send throttle state changes when this value is not specified.

@danotorrey danotorrey force-pushed the forwarder branch from 9da1e30 to 21864e9 Mar 12, 2019

Prevent race condition between Output.stop() and doGracefulShutdown()
This solves an intermittent race condition between `ForwarderOutput.doGracefulShutdown()` and the `ForwarderOutput.stop()` method. Upon server shutdown, both methods were being called. If `stop()` executed first, then the graceful shutdown was suppressed (due to the shutdownInProgress AtomicBoolean), which resulted in an incomplete shutdown.

The shutdownInProgress AtomicBoolean and IllegalStateException catch block were also removed, as they are no longer needed.
@bernd
bernd approved these changes Mar 13, 2019

@bernd bernd merged commit 5ef6a81 into master Mar 13, 2019

4 checks passed

ci-web-linter Jenkins build graylog-pr-linter-check 3463 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
graylog-project/pr Jenkins build graylog-project-pr-snapshot 3396 has succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@bernd bernd deleted the forwarder branch Mar 13, 2019

danotorrey added a commit that referenced this pull request Mar 28, 2019
Server changes for Forwarder (#5670)
* Use class-based unique metric names

Allows a subclass instance to run without throwing a "metric already exists" exceptions. This will need to be refactored later to support multiple instances. Skipping this for now to prevent CI errors in enterprise integrations repository.

* Adjust comment

* Add additional MessageOutputFactory constructor including output

This allows us to access the output ID in a backwards compatible way (does not break existing 3rd party outputs)

* Make KafkaJournal metrics unique across instances

The KafkaJournal had several hard-coded metrics directly referencing the KafkaJournal.class class name. This have been changed to allow a metricPrefix override, which can allow instances to provide unique metric names for each instance.

* Add hack for GlobalMetricName backwards compatibility

Also revert unintended formatting changes.

* Use correct oldest-segment metric name

* Refactor global oldest-segment metric name to avoid duplication

* Fix bug when checking metric name

* Revert unintended changes to KafkaJournalTest

* Teardown metrics in shutdown method

Prevents duplicate metric error message when restarting the Forwarder (via Outputs page > Edit > Save)

* Add MessageOutput.Factory2 interface for guice

New outputs need to be able to receive the Output instance via assisted
inject. We cannot just change the existing MessageOutput.Factory
interface because that would break older output plugins.

Using a "default" method for the new "create()" method also doesn't work
because guice cannot handle that and old plugins would fail to load.

The only way seems to be the introduction of a second Factory interface
that can be used by newer outputs to get the Output instance.

This change also removes "MessageOutputFactory#get(String)" because it
doesn't seem to be used anymore and it cannot work with two different
output factories.

* Add MessageOutput.Factory2 handling to OutputFacade in content-packs

* Teardown all metrics on shutdown

Several metrics were not being torn down at shutdown time. These have been cleaned up. The setup of metrics was left in two blocks (did not want to consolidate and potentially cause a situation where a metric is not created yet when used in the constructor)

* Add comment explaining the factory order in MessageOutputFactory

* Add support for disabling KafkaJournal throttling

According to a comment in graylog.conf, the original intent of the KafkaJournal.throttleThresholdPercentage field was to disable throttling updates if the value is not specified. This commit changes KafkaJournal to not send throttle state changes when this value is not specified.

* Use -1 instead of null to indicate that throttling should be disabled

* Prevent race condition between Output.stop() and doGracefulShutdown()

This solves an intermittent race condition between `ForwarderOutput.doGracefulShutdown()` and the `ForwarderOutput.stop()` method. Upon server shutdown, both methods were being called. If `stop()` executed first, then the graceful shutdown was suppressed (due to the shutdownInProgress AtomicBoolean), which resulted in an incomplete shutdown.

The shutdownInProgress AtomicBoolean and IllegalStateException catch block were also removed, as they are no longer needed.
bernd added a commit that referenced this pull request Mar 28, 2019
Server changes for Forwarder (#5670) (#5817)
* Use class-based unique metric names

Allows a subclass instance to run without throwing a "metric already exists" exceptions. This will need to be refactored later to support multiple instances. Skipping this for now to prevent CI errors in enterprise integrations repository.

* Adjust comment

* Add additional MessageOutputFactory constructor including output

This allows us to access the output ID in a backwards compatible way (does not break existing 3rd party outputs)

* Make KafkaJournal metrics unique across instances

The KafkaJournal had several hard-coded metrics directly referencing the KafkaJournal.class class name. This have been changed to allow a metricPrefix override, which can allow instances to provide unique metric names for each instance.

* Add hack for GlobalMetricName backwards compatibility

Also revert unintended formatting changes.

* Use correct oldest-segment metric name

* Refactor global oldest-segment metric name to avoid duplication

* Fix bug when checking metric name

* Revert unintended changes to KafkaJournalTest

* Teardown metrics in shutdown method

Prevents duplicate metric error message when restarting the Forwarder (via Outputs page > Edit > Save)

* Add MessageOutput.Factory2 interface for guice

New outputs need to be able to receive the Output instance via assisted
inject. We cannot just change the existing MessageOutput.Factory
interface because that would break older output plugins.

Using a "default" method for the new "create()" method also doesn't work
because guice cannot handle that and old plugins would fail to load.

The only way seems to be the introduction of a second Factory interface
that can be used by newer outputs to get the Output instance.

This change also removes "MessageOutputFactory#get(String)" because it
doesn't seem to be used anymore and it cannot work with two different
output factories.

* Add MessageOutput.Factory2 handling to OutputFacade in content-packs

* Teardown all metrics on shutdown

Several metrics were not being torn down at shutdown time. These have been cleaned up. The setup of metrics was left in two blocks (did not want to consolidate and potentially cause a situation where a metric is not created yet when used in the constructor)

* Add comment explaining the factory order in MessageOutputFactory

* Add support for disabling KafkaJournal throttling

According to a comment in graylog.conf, the original intent of the KafkaJournal.throttleThresholdPercentage field was to disable throttling updates if the value is not specified. This commit changes KafkaJournal to not send throttle state changes when this value is not specified.

* Use -1 instead of null to indicate that throttling should be disabled

* Prevent race condition between Output.stop() and doGracefulShutdown()

This solves an intermittent race condition between `ForwarderOutput.doGracefulShutdown()` and the `ForwarderOutput.stop()` method. Upon server shutdown, both methods were being called. If `stop()` executed first, then the graceful shutdown was suppressed (due to the shutdownInProgress AtomicBoolean), which resulted in an incomplete shutdown.

The shutdownInProgress AtomicBoolean and IllegalStateException catch block were also removed, as they are no longer needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.