[logs] Inherit source/service name in some cases#7310
Conversation
prognant
added
changelog/no-changelog
|
|
||
| configName := s.configName(config) | ||
| // Used to fill empty source/service in some cases | ||
| serviceName, sourceName := s.extractMainSourceAndService(configs) |
There was a problem hiding this comment.
nit: Would this be better to put in the specific block where it's used? It seems like it's only useful for a small slice of configurations so having it out here seem like it's associating it with a much larger context without a need.
There was a problem hiding this comment.
We need to run it against the whole configs slice, note that the configs slice will be rather small (like < 5) most of the time (it comes from a single annotation/label). I tried to make it more explicit.
| // cfg.Type is not overwritten as tailing a file from a Docker or Kubernetes AD configuration | ||
| // is explicitly supported (other combinations may be supported later) | ||
| cfg.Identifier = service.Identifier | ||
| // We copy service and source name from the parent container if they are not set |
There was a problem hiding this comment.
nit: Would be good to add a note about what cases can lead to the service/source name being empty on the container
There was a problem hiding this comment.
The configs slice is coming straight from user inputs, I added comment to reflect that.
| } | ||
|
|
||
| // extractMainSourceAndService extracts the source & service attached to the container config | ||
| func (s *Scheduler) extractMainSourceAndService(configs []*logsConfig.LogsConfig) (string, string) { |
There was a problem hiding this comment.
@prognant It's unclear to me when this logic is useful, and why we need to extract a source and service from the entire slice of LogsConfigs. Also I'm not sure what container config refers to (does that refer to a docker label/pod annotation, or something different?).
Could you clarify this and add a unit test that covers a scenario where this logic is useful? (and maybe refer to this unit test from code comments, so that there's a clear example of when it's useful)
There was a problem hiding this comment.
It makes sense to detail a bit this piece of code, the main use case (for now) is to handle annotation looking like :
[{"source":"mySource","service":"myService"}, {"type":"file","path":"/tmp/share/test.log"}]
And copy source/service to the file config (else it will get an empty service/source) as the foreseen behaviour was to get the same metadata attached to the lines collected from the file and the ones collected from the container.
There was a problem hiding this comment.
Interesting example: is this an annotation that is "created" by the Agent's autodiscovery logic, or does it really reflect an annotation that a user would set?
Because, to me, it is expected that your example for the logs annotation:
[{"source":"mySource","service":"myService"}, {"type":"file","path":"/tmp/share/test.log"}]would not set the source:mySource and service:myService on the file source, since IMO the 2 items in the list should be regarded as independent. IMO the annotation should be modified to:
[{"source":"mySource","service":"myService"}, {"source":"mySource","service":"myService","type":"file","path":"/tmp/share/test.log"}]for mySource and myService to be applied to the file source. This behavior is more consistent with instances annotations for example.
Am I missing something though, or a specific use case?
In any case, if the service or source defaults to "container/pod" values defined by the Agent's autodiscovery or the Logs Agent (for example the short image name, etc), then it makes sense to have the file source defined in logs annotation to default to these values as well, but I think this was already handled in your original PR (#7176).
…" (DataDog#7345) This reverts commit 8e56591.
3 participants
What does this PR do?
When tailing a file from a pod annotation (feature introduced by #7176) and the file config has no source/service it inherits the ones set for the container for consistency purpose.
Motivation
Source&service mapping consistency
Additional Notes
N/A
Describe your test plan
Same as #7176