[metricbuilder] hide details of pdata #8382
Conversation
codeboten
force-pushed
the
codeboten/refactor-metricbuilder
branch
from
faadb82 to
9570e64
Compare
cmd/mdatagen/metrics_v2.tmpl
Outdated
| } | ||
|
|
||
| // EnsureCapacity TODO | ||
| func (mb *MetricsBuilder) EnsureCapacity(length int) { |
There was a problem hiding this comment.
I don't think we need these functions. Capacity can be always taken from number of enabled metrics
There was a problem hiding this comment.
Or we can follow similar approach to how we figure out capacity for datapoints from the previous scrapes.
codeboten
force-pushed
the
codeboten/refactor-metricbuilder
branch
from
3c82b6a to
98c3fbb
Compare
| resource := pdata.NewResource() | ||
|
|
||
| for _, md := range mdata { | ||
|
|
||
| md.initializeResource(resource) |
There was a problem hiding this comment.
If I'm understanding correctly, we have a problem here.
Since the generated code is hardcoded to work with one resource, I think it is currently unable to support scrapers that actually instantiate multiple resources. Here, we're intending to create a resource per process. I believe what is actually happening in this implementation is that we're only creating one process resource, and then overwriting its attributes with each subsequent process.
There was a problem hiding this comment.
One solution would be to create another builder at the resource level, and move the Record... functions to that.
The scraper author can ask the MetricsBuilder for a new ResourceBuilder as needed, attach attributes and data points to it, and then ultimately call Emit on the MetricsBuilder. Since the MetricsBuilder created the ResourceBuilders, it can call emit on each of those in turn.
There was a problem hiding this comment.
Emit can be called with an option WithResource which copies the resource into the generate metrics, would that not work here?
There was a problem hiding this comment.
The problem is that WithResource and other parts of the generated code are all interacting with only the first resource in a slice (ResourceMetrics().At(0)).
There was a problem hiding this comment.
Ah i see.. Yes the code is limited to a single resource for each call to Emit which means the only way to support multiple resources at this time is with separate calls to emit. Let me see what this looks like with a resource builder
There was a problem hiding this comment.
I'm thinking of a bit different approach to this problem that also solves #7301. Let me submit an alternative PR and we can continue the discussion then
Refactoring the metric builder to hide details of pdata. This added a couple of methods to the metric builder interface but i think that's ok.
codeboten
force-pushed
the
codeboten/refactor-metricbuilder
branch
from
2dc134d to
f91414f
Compare
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
superseeded by #8555 |
Refactoring the metric builder to hide details of pdata. This added a couple of methods to the metric builder interface but i think that's ok.
Fix #8358
@dmitryax @djaglowski please take a look as you're familiar with the original issue.