Revert "[Agent] Remove heartbeat from bundled beats in agent." #22695
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit 39b1db8.
botelastic
bot
added
needs_team
|
Pinging @elastic/ingest-management (Team:Ingest Management) |
botelastic
bot
removed
the
needs_team
💔 Tests Failed
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪Test errors
Expand to view the tests failures
|
| Test | Results |
|---|---|
| Failed | 1 |
| Passed | 45755 |
| Skipped | 4762 |
| Total | 50518 |
Genuine test errors 
💔 There are test failures but not known flaky tests, most likely a genuine test failure.
- Name:
Build&Test / winlogbeat-windows-windows-2008-r2 / TestOpenInvalidProvider – github.com/elastic/beats/v7/winlogbeat/eventlog
|
@elasticmachine merge upstream |
shahzad31
added
the
Team:obs-ds-hosted-services
|
Pinging @elastic/uptime (Team:Uptime) |
|
if this is just to ease up development on apm side and not a product wise decision i think it would be better to focus on this issue instead #19623 or something similar so you can pack in HB on demand |
|
We need to have the bundling discussion again. I'm suggesting to first add support for heartbeat and not bundle it to unblock @shahzad31 and we will have a follow up discussion on bundling it or not as a follow up. |
|
There is a related discussion issue open regarding the packaging of Synthetics/Heartbeat/Agent |
andresrc
added
Team:Elastic-Agent
|
Pinging @elastic/agent (Team:Agent) |
|
As a note on testing, in the
Use You can unpack the |
10 tasks
The commit doesn't seem to be included in this PR, but the idea looks good to me. |
|
@urso apologies, pushed to the wrong remote, it's now updated. |
urso
reviewed
Feb 4, 2021
heartbeat/monitors/factory.go
Outdated
| @@ -52,9 +53,16 @@ type publishSettings struct { | |||
| // Output meta data settings | |||
| Pipeline string `config:"pipeline"` // ES Ingest pipeline name | |||
| Index fmtstr.EventFormatString `config:"index"` // ES output index pattern | |||
| DataStream *datastream `config:"data_stream"` | |||
| DataSet string `config:"dataset"` | |||
urso
reviewed
Feb 4, 2021
heartbeat/monitors/factory.go
Outdated
| index := fmt.Sprintf("%s-%s-%s", | ||
| settings.DataStream.Type, | ||
| settings.DataStream.Dataset, | ||
| settings.DataStream.Namespace) |
There was a problem hiding this comment.
if Type is not set you might want to set it to synthetics. Empty Dataset => generic, or empty Namespace would be default.
There was a problem hiding this comment.
Hmmmm, I actually wonder if we should just hardcode the type is there actually use-case for overriding it given that heartbeat only ever produces the one type?
|
Do we require modifications to the |
|
@urso we have not tested it yet, @justinkambic and @dominiqueclarke are working on the package and should be testing it against this branch shortly. I believe the spec should work fine here. Are there specific concerns you have? |
urso
reviewed
Feb 5, 2021
heartbeat/monitors/factory.go
Outdated
| dataset := settings.DataSet | ||
| if dataset == "" { | ||
| dataset = "uptime" | ||
| if settings.DataStream.Dataset != "" { |
There was a problem hiding this comment.
DataStream is a pointer. Do we need a nil check here?
urso
reviewed
Feb 5, 2021
heartbeat/monitors/factory.go
Outdated
|
|
||
| index := fmt.Sprintf("%s-%s-%s", | ||
| typ, | ||
| settings.DataStream.Dataset, |
There was a problem hiding this comment.
should top-level Dataset take precedence?
how about:
func defaultStr(str, def string) string {
if str != "" {
return str
}
return def
}
...
index := fmt.Sprintf("%s-%s-%s",
defaultStr(settings.DataStream.Type, "synthetics"),
defaultStr(settings.Dataset, defaultStr(settings.DataStream.Dataset, "generic")),
defaultStr(settings.DataStream.Namespace, "default")
)
urso
reviewed
Feb 5, 2021
heartbeat/monitors/factory.go
Outdated
| } | ||
| typ := settings.DataStream.Type | ||
| if typ == "" { | ||
| typ = "generic" |
There was a problem hiding this comment.
Shouldn't this be synthetics, such that the index name becomes synthetics-... by default?
There was a problem hiding this comment.
Yes, I think I misunderstood a previous discussion of ours, glad to change it
urso
reviewed
Feb 5, 2021
heartbeat/monitors/factory_test.go
Outdated
| Version: "8.0.0", | ||
| } | ||
| tests := []struct { | ||
| name string |
There was a problem hiding this comment.
nit, remove name and:
tests := map[string]struct { ... }
urso
approved these changes
Feb 8, 2021
|
/test |
andrewvc
added a commit
that referenced
this pull request
Feb 9, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adding heartbeat into agent, since we are restarting work on fleet integration with uptime/heartbeat in 7.11
Reverts #21602