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

[Heartbeat] Incorporate factory metadata for autodiscover #10258

Merged
merged 3 commits into from
Jan 24, 2019

Conversation

andrewvc
Copy link
Contributor

Heartbeat factories get metadata from autodiscover and other sources.

This change automatically adds that data to events keeping heartbeat behavior in-line with other beats.

Heartbeat factories get metadata from autodiscover and other sources.

This change automatically adds that data to events keeping heartbeat behavior in-line with other beats.
@andrewvc andrewvc added enhancement in progress Pull request is currently in progress. Heartbeat Team:obs-ds-hosted-services Label for the Observability Hosted Services team labels Jan 22, 2019
@andrewvc andrewvc self-assigned this Jan 22, 2019
@andrewvc andrewvc requested a review from a team as a code owner January 22, 2019 18:03
@elasticmachine
Copy link
Collaborator

Pinging @elastic/uptime

@andrewvc andrewvc added review and removed in progress Pull request is currently in progress. labels Jan 22, 2019
@andrewvc
Copy link
Contributor Author

@ruflin my original thinking was that the additional fields here might be considered 'breaking', but I no longer believe that. I think that was backwards. I've added the needs_backport label. LMK if you think that isn't deserved. I think it should make it into 6.7 if possible.

@andrewvc andrewvc added the needs_backport PR is waiting to be backported to other branches. label Jan 22, 2019
Copy link
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Change LGTM and agree this should not be a breaking change. Could you add a changelog entry?

@exekias Can you have also a look? I'm mainly surprised how simple the change is and if we are missing something.

@ruflin ruflin requested a review from exekias January 23, 2019 07:22
@andrewvc
Copy link
Contributor Author

@ruflin added changelog entry (and fixed a previous one that was in the wrong spot)

@ruflin
Copy link
Contributor

ruflin commented Jan 24, 2019

jenkins, test this

@@ -112,6 +112,7 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d
*Heartbeat*

- Made monitors.d configuration part of the default config. {pull}9004[9004]
- Fixed rare issue where TLS connections to endpoints with x509 certificates missing either notBefore or notAfter would cause the check to fail with a stacktrace. {pull}9566[9566]
Copy link
Contributor

Choose a reason for hiding this comment

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

Does not seem to belong to this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically no, but I moved it since it was in the wrong spot. Should I make a new PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just realised now that you moved it only and was not added. All good.

@andrewvc andrewvc merged commit 8f4e186 into elastic:master Jan 24, 2019
@andrewvc andrewvc added v6.7.0 and removed needs_backport PR is waiting to be backported to other branches. labels Jan 24, 2019
t.client, err = t.monitor.pipelineConnector.ConnectWith(beat.ClientConfig{
EventMetadata: t.config.EventMetadata,
Processor: t.processors,
Fields: common.MapStr{"event": common.MapStr{"dataset": "uptime"}},
Fields: fields,
Copy link
Contributor

Choose a reason for hiding this comment

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

there is a DynamicFields here in ClientConfig, were you can pass t.monitor.factoryMetadata directly, no need for that DeepUpdate

Copy link
Contributor

@exekias exekias left a comment

Choose a reason for hiding this comment

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

left a comment that I think needs to be addressed, but this is looking awesome!

andrewvc added a commit to andrewvc/beats that referenced this pull request Jan 29, 2019
)

[Heartbeat] Incorporate factory metadata for autodiscover

Heartbeat factories get metadata from autodiscover and other sources.

This change automatically adds that data to events keeping heartbeat behavior in-line with other beats.

(cherry picked from commit 8f4e186)
andrewvc added a commit that referenced this pull request Jan 30, 2019
…10331)

[Heartbeat] Incorporate factory metadata for autodiscover

Heartbeat factories get metadata from autodiscover and other sources.

This change automatically adds that data to events keeping heartbeat behavior in-line with other beats.

(cherry picked from commit 8f4e186)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants