Skip to content

Conversation

@cachedout
Copy link
Contributor

One of two PRs necessary to address elastic/logstash#10602

@ycombinator
Copy link
Contributor

@cachedout I've never built an LS plugin and tested it in conjunction with another LS core PR. Would you mind posting some instructions on how I can essentially run this PR along with elastic/logstash#10561? Thanks!

@cachedout
Copy link
Contributor Author

@ycombinator I did my development by editing the installed gem directly, the instructions here should allow for testing in your environment after checking out this PR into a local branch: https://github.com/logstash-plugins/logstash-output-elasticsearch/blob/master/README.md#2-running-your-unpublished-plugin-in-logstash

Copy link
Contributor

@yaauie yaauie left a comment

Choose a reason for hiding this comment

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

After several read-throughs, it appears as though this and the other PR accomplish the goal, but do so in a way that is fragile and bound to surprise developers in the future, exploiting Settings as a key/value store by means of dynamically creating settings with specific default values that are otherwise unreachable.

I realise that it can be frustrating to try to wire features through multiple components of our ecosystem, and would like to support you in any way that I can to reduce that frustration. I will follow up momentarily with a PR on Logstash that would add a per-plugin metadata store that could be used to this effect, along with instructions for its use.

@yaauie
Copy link
Contributor

yaauie commented Apr 4, 2019

Relevant PR on Logstash core is elastic/logstash#10636

With it, the implementation of LogStash::Outputs::ElasticSearch::Common#discover_cluster_uuid becomes something like:

    def discover_cluster_uuid
      if defined?(plugin_metadata)
        cluster_info = @client.get('/')
        plugin_metadata.set(:cluster_uuid, cluster_info['cluster_uuid'])
      end
    end

And extracting this metadata in your other PR becomes something like:

  def resolve_cluster_uuids
    outputs.each_with_object(Set.new) do |output, cluster_uuids|
      if LogStash::PluginMetadata.exists?(output.id)
        cluster_uuids << LogStash::PluginMetadata.for_plugin(output.id).get(:cluster_uuid)
      end
    end.to_a.compact
  end

@cachedout
Copy link
Contributor Author

jenkins, re-test this

@cachedout cachedout closed this Apr 29, 2019
@cachedout cachedout reopened this Apr 29, 2019
@cachedout cachedout force-pushed the cluster_uuid_lookup branch from 46748e5 to b7b3ef2 Compare April 29, 2019 14:56
The DLQ tests were written to just assume a clean buffer in the logs
but this is not a safe assumption.

Now we temporarily replace the logging subsystem with a double
during these tests so that we can be assured that we are checking
a clean instance.
@cachedout
Copy link
Contributor Author

The failing tests also appear to be failing on the master branch for me.

I had to also fix up some other tests which were failing because they depended on a certain sequence of log events instead of being fully isolated.

@yaauie can you re-review this?

Copy link
Contributor

@yaauie yaauie left a comment

Choose a reason for hiding this comment

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

I'm sorry for missing the request to re-review. I agree with @jsvd and think this'll be ready to go as soon as the minor refactor of the discover_cluster_uuid is accomplished.

@cachedout
Copy link
Contributor Author

Changes pushed. This is still failing a local test but I believe this is a problem unrelated to this PR. It also fails in the master branch and is due to a problem that occurs when attempting to fetch the ES version with a path specified.

As such, I believe this is ready for merge.

Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

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

LGTM. Rebased, bumped and merged in 181ec8a so closing this PR

Publishing as 10.1.0

@jsvd jsvd closed this May 6, 2019
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.

4 participants