Collector Reporter #208
Collector Reporter #208
Conversation
dmathieu
force-pushed
the
collector-reporter
branch
from
2c97742
to
59e075f
Compare
dmathieu
force-pushed
the
collector-reporter
branch
from
b23cc98
to
d03c2d9
Compare
|
I've done this as close as possible from what the OTLP reporter does. |
dmathieu
force-pushed
the
collector-reporter
branch
from
cf60197
to
3736518
Compare
|
This adds a lot of code duplication, |
|
While the way it works is very similar, the data types are very different. We need to use the pdata data types for communicate with the collector. Ultimately, the OTLP reporter should also disappear. |
That makes sense, hopefully in a relatively near future :) |
|
@open-telemetry/ebpf-profiler-approvers this is ready for review. |
| // Next step: Dynamically configure the size of this LRU. | ||
| // Currently, we use the length of the JSON array in | ||
| // hostmetadata/hostmetadata.json. | ||
| hostmetadata, err := lru.NewSynced[string, string](115, hashString) |
There was a problem hiding this comment.
Not sure what the rationale was for choosing an LRU here instead of a map 🤔
We don't have to change it in this PR, though.
There was a problem hiding this comment.
It's just the same than we use in the OTLP reporter.
There was a problem hiding this comment.
I know, same thoughts on that code. Here, we just copied technical debt. But I am just raising this for awareness, it's not meant as a blocker.
handling build ID kind is complicated at the moment, due to the use of internal types, and these fields are removed from the proto anyway
Co-authored-by: Tim Rühsen <[email protected]>
dmathieu
force-pushed
the
collector-reporter
branch
from
5efd85c
to
d9e97f3
Compare
Co-authored-by: Tim Rühsen <[email protected]>
Co-authored-by: Tim Rühsen <[email protected]>
|
|
||
| // ReportTraceEvent enqueues reported trace events for the Collector reporter. | ||
| func (r *CollectorReporter) ReportTraceEvent(trace *libpf.Trace, meta *TraceEventMeta) { | ||
| if r.nextConsumer == nil { |
There was a problem hiding this comment.
I'm wondering, if this should generate a log in some way. So far, there can only be a single reporter. If CollectorReporter is configured as such single reporter, then silently returning here will result in massive data/information loss without notice.
There was a problem hiding this comment.
This case can only happen in tests, where we mock structs. At runtime, the collector will refuse to boot if the data is exported nowhere.
| } | ||
|
|
||
| // lookupCgroupv2 returns the cgroupv2 ID for pid. | ||
| func (r *CollectorReporter) lookupCgroupv2(pid libpf.PID) (string, error) { |
There was a problem hiding this comment.
This is a copy of func (r *OTLPReporter) lookupCgroupv2(pid libpf.PID) (string, error) {..}. Can we reduce duplicate code?
In both implementations the reporter details matter less and the common ground is the use of cgroupv2ID *lru.SyncedLRU[libpf.PID, string].
reporter/collector_reporter.go
Outdated
|
|
||
| // addPdataProfileAttributes adds attributes to Profile.attribute_table and returns | ||
| // the indices to these attributes. | ||
| func addPdataProfileAttributes(profile pprofile.Profile, |
There was a problem hiding this comment.
With code duplication we miss out on changes like #212 in some parts.
Co-authored-by: Florian Lehner <[email protected]>
| case int64: | ||
| strVal := strconv.FormatInt(val, 10) | ||
| attributeCompositeKey = attr.key + "_" + strVal | ||
| attributeValue = strVal |
There was a problem hiding this comment.
While #212 differentiate between string and int64 this now encodes again everything as string. process.PID are considered to be a numeric value by OTel SemConv, they should be added as int64 to the AttributeTable instead of string.
florianl
force-pushed
the
collector-reporter
branch
from
3d14426
to
f8f2ab7
Compare
This sets up a new reporter that allows passing data to a collector consumer.