Fix metrics destination index and dataset to match v1 #2004
Conversation
joshdover
added
bug
joshdover
requested review from
aleksmaus and
michalpristas
and removed request for
a team
| @@ -596,20 +596,20 @@ func (b *BeatsMonitor) injectMetricsInput(cfg map[string]interface{}, componentI | |||
| idKey: "metrics-monitoring-" + name, | |||
| "data_stream": map[string]interface{}{ | |||
| "type": "metrics", | |||
| "dataset": fmt.Sprintf("elastic_agent.%s", name), | |||
| "dataset": fmt.Sprintf("elastic_agent.%s", binaryName), | |||
There was a problem hiding this comment.
We did do this change before as well: https://github.com/elastic/elastic-agent/pull/1854/files
Could you just change name := strings.ReplaceAll(strings.ReplaceAll(unit, "-", "_"), "/", "_") // conform with index naming policy above instead to fix this?
Specifically using strings.ReplaceAll(binaryName instead of strings.ReplaceAll(unit
There was a problem hiding this comment.
Here is this suggested change as a forward-port PR: #2006
There was a problem hiding this comment.
Hmm I find doing it this way curious. I believe it will result in multiple streams with the same idKey, which I presumed would be an issue.
There was a problem hiding this comment.
I believe it will result in multiple streams with the same idKey, which I presumed would be an issue.
I'm not very familiar with the constraints on index names here, but the binary name can be anything that is a valid file name so I could believe we have to do some sanitization/normalization on it.
We did the exact same change for logs on main and 8.6 in https://github.com/elastic/elastic-agent/pull/1845/files so if we decide to change it we should change it in both places.
There was a problem hiding this comment.
Merged the backport to fix main quickly. We can still adjust the way this is done if needed.
|
Aha, #1854 was not forward ported to main it was merged directly to 8.6 |
|
The logs change was forward ported #1845 |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
🌐 Coverage report
|
|
Closing this for now, no reported issues with current implementation and fix was forwardported to main |
What does this PR do?
Fixes the data streams that we send agent monitoring metrics to use the
elastic_agent.<binary name>pattern from v1 instead ofelastic_agent.<unit id>Why is it important?
We need to be sure agent's monitoring metrics get delivered so they must match what's defined by the elastic_agent package. Also, having a data stream per unique unit will cause an explosion of data streams without user benefit. Without this change, metrics from individual binaries are being dropped with errors like:
This is the same change we did for logging in #1845 and the long-term plan for v2 logs and metrics will be discussed in:
Checklist
CHANGELOG.next.asciidocorCHANGELOG-developer.next.asciidoc.Author's Checklist
How to test this PR locally
Enroll an agent with monitoring enabled, verify that none of the error logs above show up in
logs-elastic_agent.metricbeat-defaultand verify that metrics are available inmetrics-elastic_agent.metricbeat-default