-
Notifications
You must be signed in to change notification settings - Fork 487
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
[Tempo] Automatic Logging #551
Conversation
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
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.
Fancy! I have some general concerns about how this interacts with the reloading system.
if lokiInstance == nil { | ||
return fmt.Errorf("loki instance %s not found", p.cfg.LokiName) | ||
} | ||
p.lokiChan = lokiInstance.Promtail().Client().Chan() |
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.
This isn't going to work if the promtail restarts 😞 We'll need this to use a channel on the instance (which should be immutable) and have the Loki instance forward from the active promtail over the instance's channel.
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.
Yeah, I did some mild testing and it seemed to work, but my expectation is that in the general case it would not. I was considering:
- Shutdown Tempo subsystem
- Reload Loki
- Reload Tempo
I think this would work.
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.
It'd work, but I'd prefer to try to get this to work with the existing ApplyConfig mechanism and not introducing shutdowns if we can. What were your concerns with forwarding the channel?
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.
@rfratto have a first pass attempt at doing this with the ApplyConfig
. Let me know what you think.
pkg/tempo/automaticloggingprocessor/automaticloggingprocessor.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
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.
Nice! LGTM 👍
pkg/tempo/automaticloggingprocessor/automaticloggingprocessor.go
Outdated
Show resolved
Hide resolved
pkg/tempo/automaticloggingprocessor/automaticloggingprocessor.go
Outdated
Show resolved
Hide resolved
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.
Nice! Mostly nits about the docs.
return | ||
} | ||
|
||
sent := p.lokiInstance.SendEntry(api.Entry{ |
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.
In the future we'll have to be careful to make sure the Loki instances don't get replaced so this doesn't end up with a stale reference. I think we're fine for now though.
We could be defensive and perform the lookup each time, but I'm not sure it's worth it just yet.
@@ -2045,6 +2045,22 @@ remote_write: | |||
[ sending_queue: <otlpexporter.sending_queue> ] | |||
[ retry_on_failure: <otlpexporter.retry_on_failure> ] | |||
|
|||
# automatically log lines to Loki for discovery/metrics | |||
automatic_logging: |
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.
Just curious: what enables this? Is it defining automatic_logging
in the config or is it providing a value for loki_name
? Can we document which?
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.
Adding automatic_logging
will instantiate the processor, but it will fail with an error message if it can't find the provided loki_name
.
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
* garbage POC, let's goooooooooooOOO! Signed-off-by: Joe Elliott <number101010@gmail.com> * Added basic configuration Signed-off-by: Joe Elliott <number101010@gmail.com> * Added skeleton processor Signed-off-by: Joe Elliott <number101010@gmail.com> * first pass config wiring Signed-off-by: Joe Elliott <number101010@gmail.com> * got things to work? Signed-off-by: Joe Elliott <number101010@gmail.com> * kinda sorta works Signed-off-by: Joe Elliott <number101010@gmail.com> * made things work Signed-off-by: Joe Elliott <number101010@gmail.com> * fixed-ish dur. upped loki and grafana Signed-off-by: Joe Elliott <number101010@gmail.com> * fixed duration, added svc name, did processes correctly Signed-off-by: Joe Elliott <number101010@gmail.com> * Added tempo Signed-off-by: Joe Elliott <number101010@gmail.com> * configurable tags Signed-off-by: Joe Elliott <number101010@gmail.com> * config Signed-off-by: Joe Elliott <number101010@gmail.com> * Addd stopping flag Signed-off-by: Joe Elliott <number101010@gmail.com> * Added tests Signed-off-by: Joe Elliott <number101010@gmail.com> * Added configurable stuff Signed-off-by: Joe Elliott <number101010@gmail.com> * Added automatic logging to docs Signed-off-by: Joe Elliott <number101010@gmail.com> * added otel config test Signed-off-by: Joe Elliott <number101010@gmail.com> * Made tid configurable Signed-off-by: Joe Elliott <number101010@gmail.com> * changelog Signed-off-by: Joe Elliott <number101010@gmail.com> * Removed zero valued defaults Signed-off-by: Joe Elliott <number101010@gmail.com> * Put changelog entry in the proper spot Signed-off-by: Joe Elliott <number101010@gmail.com> * - _processor Signed-off-by: Joe Elliott <number101010@gmail.com> * fixed tests Signed-off-by: Joe Elliott <number101010@gmail.com> * Added cross config validation Signed-off-by: Joe Elliott <number101010@gmail.com> * review suggestions Signed-off-by: Joe Elliott <number101010@gmail.com> * First pass SendEntry Signed-off-by: Joe Elliott <number101010@gmail.com> * lint Signed-off-by: Joe Elliott <number101010@gmail.com> * Added configurable timeout Signed-off-by: Joe Elliott <number101010@gmail.com> * Addd logging message when bad things happen Signed-off-by: Joe Elliott <number101010@gmail.com> * Actually check sent :P Signed-off-by: Joe Elliott <number101010@gmail.com> * review cleanup Signed-off-by: Joe Elliott <number101010@gmail.com> * config updates Signed-off-by: Joe Elliott <number101010@gmail.com> * duh Signed-off-by: Joe Elliott <number101010@gmail.com>
PR Description
Adds an automatic logging processor, documentations, tests and updates the docker-compose example to use it.
Example lines:
Which issue(s) this PR fixes
Notes to the Reviewer
Also updated Grafana, Loki and added Tempo to the default docker-compose example.
PR Checklist