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

fix(docker_logs source): fix docker logs host key #6552

Conversation

drunkirishcoder
Copy link
Contributor

@drunkirishcoder drunkirishcoder commented Feb 24, 2021

fixes #6394 for real this time.

The problem with the last attempted fix #6395 was when serde deserializes the configuration for docker_logs, the lazy static LOGSCHEMA hasn't been constructed yet because that also relies on reading from the same configuration file to get the proper values. Basically it's an order of operations problem.

This time, we delay retrieving log_schema().host_key() till much later, when the lazy static had the chance to be initialized. Tested with a local build and this works. (I'm 99.99% sure I think 😄 )

@drunkirishcoder drunkirishcoder requested review from a team and prognant and removed request for a team February 24, 2021 22:19
@drunkirishcoder drunkirishcoder changed the title fix docker logs host key fix(docker_logs source): fix docker logs host key Feb 24, 2021
Signed-off-by: drunkirishcoder <[email protected]>
@drunkirishcoder drunkirishcoder force-pushed the djin/docker-logs-host-key branch from 6a87957 to a9bc115 Compare February 24, 2021 22:20
@jszwedko
Copy link
Member

@drunkirishcoder aha, thanks for the explanation! It seems like we do this in a few other places as well:

$ grep -R -e 'host_key: ' -e 'timestamp_key: ' -B 1  src | grep -e 'serde.*default' -A 1 | grep -v log_schema
--
--
src/sinks/humio/metrics.rs-    #[serde(default = "default_host_key")]
src/sinks/humio/metrics.rs:    host_key: String,
--
src/sinks/humio/logs.rs-    #[serde(default = "default_host_key")]
src/sinks/humio/logs.rs:    pub(in crate::sinks::humio) host_key: String,
--
src/sinks/splunk_hec.rs-    #[serde(default = "default_host_key")]
src/sinks/splunk_hec.rs:    pub host_key: String,
--
src/sources/docker_logs.rs-    #[serde(default = "default_host_key")]
src/sources/docker_logs.rs:    host_key: String,

Should we update them as well? I can see this would be an easy mistake to make too. Maybe we could guard against it somehow?

@drunkirishcoder
Copy link
Contributor Author

@jszwedko yep you are right, they all have the same problem of not getting the overridden host_key. only difference is they are all sinks so they are not generating events. but that probably shouldn't matter. I can update them. agree it would be nice to guard against this. but I'm not sure how. obviously you don't want this to be a run time error.

@ktff
Copy link
Contributor

ktff commented Feb 26, 2021

There is a more general solution for this of deserializing the config once on startup to set all the globals, and then deserializing it again like usual. That way we could avoid this problem completely and not relay on us knowing/remembering that this problem exists. Implementation shouldn't be too complex, we just need to deserialize from the same bytes to avoid a bunch of corner cases.

@drunkirishcoder
Copy link
Contributor Author

Ok I will hold off on making change to humio and splunk sinks then and wait for a more generalized solution to fix them. I'mnot familiar enough with the config loading process. don't want to try and mess everything up somehow. note, those sinks still all have a problem with not reading from the overridden value.

LogSchema::default().host_key().to_string()

instead should be reading from

log_schema().host_key().to_string()

@ktff
Copy link
Contributor

ktff commented Feb 26, 2021

Ok, I'll make the change, and also look into if we can avoid LogSchema::default().host_key().to_string() and it's variants being used in such way.

@jszwedko
Copy link
Member

👍 thanks both. I like the idea of deserializing just the globals first. That should avoid inadvertently hitting this sort of issue.

@ktff ktff mentioned this pull request Mar 1, 2021
3 tasks
@ktff
Copy link
Contributor

ktff commented Mar 3, 2021

Closing in favor of #6581

@ktff ktff closed this Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docker_logs not using the host key defined in the configuration
3 participants