-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[Beats] Adding support for elastic-agent-autodiscovery v0.6.13 Introduce log message for not supported annotations #38213
Conversation
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
@@ -30,6 +30,8 @@ import ( | |||
"github.com/elastic/elastic-agent-libs/logp" | |||
) | |||
|
|||
var AllSupportedHints = []string{"enabled", "module", "metricsets", "host", "period", "timeout", "metricspath", "username", "password", "stream", "processors", "multiline", "json", "disable", "ssl", "metrics_filters", "raw", "include_lines", "exclude_lines", "fileset", "pipeline"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does metricspath
actually works? shouldn't it be metrics_path
based on https://www.elastic.co/guide/en/beats/metricbeat/current/configuration-autodiscover-hints.html#_co_elastic_metricsmetrics_path ?
where disable
is coming from?
Also as I understand it is a merged list for the metricbeat and for the filebeat, correct? can you please add a comment for that? Is there any way to split it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed metricspath to metrics_path ! Nice catch, thanks Tania!
Disable comes from this: https://github.com/elastic/elastic-agent-autodiscover/blob/7490558445cb8d3248462255edc8ac64bc18b8eb/utils/hints_test.go#L177
We can disable logs collection apparently
I have added a comment in 7d2c429
Currently we parse annotations based on the fact that start with the prefix (L209) and then we split with "/" symbol (L215)
So currently this is a unified solution for elastic-agent and beats. I deliberately had moved the solution to the elastic-agent-autodiscovery in oder not to introduce an additional annotation loop and different implementations. So splitting it for beats will require additional checks if we have Prefix.logs or Prefix.metrics. I consider it out of scope here, mainly because the main reason behind those changes, is to offer the feature in elastic-agent and not in beats. Hope it makes sense
@@ -383,7 +383,7 @@ func (d *Provider) generateHints(event bus.Event) bus.Event { | |||
e["ports"] = ports | |||
} | |||
if labels, err := dockerMeta.GetValue("labels"); err == nil { | |||
hints := utils.GenerateHints(labels.(mapstr.M), "", d.config.Prefix) | |||
hints, _ := utils.GenerateHints(labels.(mapstr.M), "", d.config.Prefix, []string{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't know we call GenerateHints
here. Looking on this use case, wondering if it wouldn't be a better option to introduce additionally smth like GenerateHintsWithValidation
instead - maybe we should consider it in future for cases when signature is changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here GenerateHints
will actually return all hints as incorrect but we just ignore them. I think we should make it more clear. Maybe add a bool (validate
), which is then checked in GenerateHints
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is still one more linter issue to be resolved but the rest looks good to me.
@rdner I added the skip based on comment becaus this is a test f3b6f21#diff-9f24ba274864b933a726842c2b9fb8ee487a13545579938f9e373d0463611142R2200 |
@rdner have you seen this https://github.com/elastic/beats/actions/runs/8813847492/job/24193021847?pr=38213 again? |
@elastic/beats-tech-leads can I have a review here as well? |
This pull request is now in conflicts. Could you fix it? 🙏
|
@gizas Looks like something is wrong with the Ubuntu itself, it failed on buildkite too. I think we should try to restart, I'm going to update the branch. |
@rdner this looks better now. I tried to upddate the go.mod and align it as well. The failing tests (mainly related to Windows) I think are not related with changes here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codeowner approval.
Note: Dont merge until elastic/elastic-agent-autodiscover#81 is merged.PR is mergedWe need to udpate go.mod of the beats with v0.6.9
Proposed commit message
Relates with the updates of shared elastic/elastic-agent-autodiscover#81.
In more details we updated
utils.GenerateHints
to be able to check whether the provided annotation follows the supported format and vocabulary. The check happens for annotations that have prefix co.elasticChecklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.How to test this PR locally
Related issues
Logs