reporter: use tree to populate pprofile.Profiles#528
Conversation
|
|
||
| // TraceEventsTree stores samples and their related metadata in a tree-like | ||
| // structure optimized for the OTel Profiling protocol representation. | ||
| type TraceEventsTree map[ContainerID]map[libpf.Origin]KeyToEventMapping |
There was a problem hiding this comment.
TraceEventsTree is very generic.
Since it's a grouping by container, maybe name it more specific like TraceEventsByContainer or similar.
There was a problem hiding this comment.
Naming is hard and suggestions are welcome.
I do think, TraceEventsByContainer is not the best name, as first level of the nested maps, maps ContainerID to libpf.Origin. From TraceEventsByContainer I would expect to get a faster/direct access to a trace event, once I have a container ID.
There was a problem hiding this comment.
One idea is to have a dedicated type for map[libpf.Origin]KeyToEventMapping. That should make it easier to find a better name for TraceEventsTree.
There was a problem hiding this comment.
These indirections in namings will make the code less readable and harder to follow. In particular if every nested level gets their own type name.
It does make sense to use dedicated subnames for some part of nested maps, if these parts are used outside of the scope of the tree. But I think this is not the case here.
| } | ||
|
|
||
| // mkProfileID creates a random profile ID. | ||
| func mkProfileID() []byte { |
There was a problem hiding this comment.
Dropped this function already in compliance with open-telemetry/opentelemetry-proto#665.
| // Do not append empty profiles, if there | ||
| // is not profiling data for this origin. | ||
|
|
||
| for containerID, originToEvents := range tree { |
There was a problem hiding this comment.
This will probably aggravate the behavior reported in #534, should it be looked at before this PR is merged?
| // Do not append empty profiles, if there | ||
| // is not profiling data for this origin. | ||
|
|
||
| for containerID, originToEvents := range tree { |
| intervals := times.New(cfg.ReporterInterval, | ||
| cfg.MonitorInterval, cfg.ProbabilisticInterval) | ||
|
|
||
| kernelVersion, err := helpers.GetKernelVersion() |
There was a problem hiding this comment.
What is the plan for these (and hostID) ?
There was a problem hiding this comment.
Like for the additional k8s attributes, the recommended way is to use OTel processors to enrich the data. Here is an example to enrich ResourceProfile with information like host.id:
processors:
resourcedetection:
detectors: [ system ]
system:
hostname_sources: [ "os" ]
resource_attributes:
host.id:
enabled: true
host.name:
enabled: true
I'm well aware of discussions like open-telemetry/semantic-conventions#581. Dropping here the costum host.id and using OTel processors to gather it, aligns with OTel and simplifies correlation.
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
|
I had to force push to the branch in order to resolve merge conflicts (in |
4 participants
Fixes #517