Nebo15 / logger_json

JSON logger formatter with support for Google Cloud, DataDog and other for Elixir.
https://nebo15.github.io/logger_json/
MIT License
237 stars 92 forks source link

feat: allow setting Datadog syslog.hostname attribute #87

Closed btkostner closed 2 years ago

btkostner commented 2 years ago

What is breaking

When logging to Datadog from a kubernetes cluster, the syslog hostname is set to the container hostname. This means Datadog will try to use the container hostname as the actual host name for metrics, traces, etc. Because of this, nothing will link up successfully. Sadly you can't rewrite that or do some sort of pipeline fix for this.

Why this change fixes it

Removing the syslog hostname means that dd-agent will fall back to its native hostname logic. This will pick up the same hostname when running on a regular server (non containerized), and it will pick up the correct (node) host name when running in a container (kubernetes).

coveralls commented 2 years ago

Coverage Status

Coverage increased (+0.2%) to 75.494% when pulling fe266288e65a97b577409b6202260856e2b98a71 on btkostner:patch-1 into 349c8174886135a02bb16317f76beac89d1aa20d on Nebo15:master.

AndrewDryga commented 2 years ago

Hello @btkostner, I'm not sure about this one because it would be a breaking change for users of DataDog agents that rely on a hostname to be an Erlang one. How DD-agent hostname is different? Are you sure that we can't read it in the same way as DD-agent does and give you an option to override the hostname?

btkostner commented 2 years ago

The only time it differs is when running Elixir in a container, because it grabs the container hostname instead of the host's hostname. Sadly this isn't something I can overwrite in the agent configuration.

AndrewDryga commented 2 years ago

@btkostner do you run it in Kubernetes or some other environment?

btkostner commented 2 years ago

@AndrewDryga Yes, this is in kubernetes. I can show some screenshots in Datadog of some different setups if you want more clarity of what's happening.

AndrewDryga commented 2 years ago

@btkostner I think there is a way to do this without introducing breaking changes: you can expose the hostname using an environment variable and then read it and pass it to the logger (https://kubernetes.io/docs/tasks/inject-data-application/environment-variable-expose-pod-information/) so that both DD-agent values and Logger one would be equal. In logger we can allow overriding the host with anything you want.

btkostner commented 2 years ago

@AndrewDryga That would work. We could also just make a bool to include the hostname and have it default to true. Both options work and I can update the PR with the solution. Which one would you prefer?

AndrewDryga commented 2 years ago

@btkostner Great! I can share the plan that I have in my head that should fit all the use-cases:

We can add a formater_state here and a formatter_opts to the configuration. It would allow passing any formatter-specific options when the logger is started and make sure that people won't do expensive calls for each of the log entries. It can be used like so (eg. in release.exs):

config :logger_json, :backend,
  json_encoder: Jason,
  formatter: LoggerJSON.Formatters.GoogleCloudLogger,
  formatter_opts: [hostname: System.get_env("KUBERNETES_NODE_HOSTNAME")]

To make it work we probably would need to extend the behavior (https://github.com/Nebo15/logger_json/blob/master/lib/logger_json/formatter.ex) and add an init callback that will receive formatter_opts and return formatter_state which would be saved and passed to format_event. New init callback should be called somewhere here to make sure it would support runtime reconfiguration.

When the backend initializes we can check if such an option was given and:

AndrewDryga commented 2 years ago

I know it shoulds like a lot, if you feel like it's too much I'll handle this myself but a bit later

btkostner commented 2 years ago

That looks like a solid well thought out plan. I'll start working on it today :+1:

btkostner commented 2 years ago

Alrighty, PR is updated. Let me know how it looks :+1:

btkostner commented 2 years ago

As a stop gap, I found a way to unset that value in Datadog itself. You can remove syslog.hostname from the hostname attributes by editing the "Preprocessing for JSON logs" pipeline.

Screen Shot 2022-06-20 at 8 17 45 PM
AndrewDryga commented 2 years ago

Hey @btkostner sorry for taking it so long, was distracted by life and work :). I think this still is a great change that can be used by other providers so let's merge it anyways, even with a workaround.

btkostner commented 2 years ago

@AndrewDryga No problem. I figured there was more important stuff happening in your life. No need to apologize.

AndrewDryga commented 2 years ago

Thank you @btkostner, great work! Can you run this in your project and verify it's what you need? I'll release a new hex version once I get feedback from you and make sure it's stable.

btkostner commented 2 years ago

Alright. Aside from the small doc fix I opened above, it looks to be working for me. I'm going to put it up on our staging cluster to make sure there are no side effects, but it should be good from my end.