-
Notifications
You must be signed in to change notification settings - Fork 265
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
Add live graph backend (merge to feature branch) #2645
base: alloy-live-graph
Are you sure you want to change the base?
Conversation
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.
Looking good! some comments, but nothing big
internal/component/otelcol/internal/lazyconsumer/lazyconsumer.go
Outdated
Show resolved
Hide resolved
internal/component/otelcol/internal/lazyconsumer/lazyconsumer_test.go
Outdated
Show resolved
Hide resolved
internal/component/otelcol/internal/livedebuggingconsumer/livedebuggingconsumer.go
Outdated
Show resolved
Hide resolved
internal/component/otelcol/internal/livedebuggingconsumer/livedebuggingconsumer.go
Outdated
Show resolved
Hide resolved
a79c730
to
0dd571d
Compare
internal/component/otelcol/internal/livedebuggingconsumer/livedebuggingconsumer.go
Outdated
Show resolved
Hide resolved
internal/component/otelcol/internal/livedebuggingconsumer/livedebuggingconsumer.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.
Looks much, much cleaner without the callbacks to LiveDebugging(...)
function from the service. Few more comments, but we're almost there!
internal/component/otelcol/internal/interceptorconsumer/logs.go
Outdated
Show resolved
Hide resolved
internal/component/otelcol/internal/interceptorconsumer/logs.go
Outdated
Show resolved
Hide resolved
"go.opentelemetry.io/collector/pdata/plog" | ||
) | ||
|
||
type LogsInterceptorFunc func(context.Context, plog.Logs) error |
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.
For all the 3 telemetry signals, can we use Go generics to simplify the code here? Seems like the variation would be only really on plogs.Logs
and otelconsumer.Logs
types.
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.
that works for plogs.Logs but I don't think that we can use a generic type for otelconsumers.Logs because we call ConsumeLogs with it. That means we will still need an interceptor type for each telemetry signal and each will still have its corresponding ConsumeMetrics|ConsumeLogs|ConsumeTraces func
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 see. Maybe that could be avoided with a func
but if not, then we'll need to live with this duplication :(
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 couldn't find a way to reduce the duplication while keeping the complexity of the code low. Having a bit of duplication for Otel (whether it's in the Otel col project or in Alloy) in the logic between metrics/logs/traces seems to be common. (see the fanout consumer for example). Maybe we should keep it simple and follow the same pattern with a bit of duplication?
internal/component/otelcol/internal/livedebuggingpublisher/livedebuggingpublisher.go
Outdated
Show resolved
Hide resolved
return ids | ||
} | ||
|
||
func PublishLogsIfActive(debugDataPublisher livedebugging.DebugDataPublisher, componentID string, ld plog.Logs, nextLogs []otelcol.Consumer) { |
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 don't think nextLogs
needs to be otelcol.Consumer
, you only ever use them to extract component ID.
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.
True but it's quite handy for the components because it's easy to provide.
Would you prefer that I change it to:
PublishLogsIfActive(debugDataPublisher livedebugging.DebugDataPublisher, componentID string, ld plog.Logs, componentIDs []string)
and that I export the extractIDs function?
Or do you have a different idea in mind? I'd like to keep the code as little intrusive as possible in the components
PR Description
This is the backend part of the new live graph.
The approach is similar to live debugging, except that it adds the callback to all components in the given module.
The live debugging feed is now a struct that contains data needed to build the new graph. Because the data string is not needed for the graph, it's passed around as a function. This way we can use the same struct for the live debugging and the live graph with limited performance overhead.
Which issue(s) this PR fixes
Fixes #2608
Notes to the Reviewer
This was tested manually via curl:
curl -N http://localhost:12345/api/v0/web/graph
to get the data from all components at the rootcurl -N http://localhost:12345/api/v0/web/graph/{moduleID}
to get data from a particular modulePR Checklist