contrib/iinternal/coreinternal: add helpers for Pdata Metrics

[odeke-em]

Adds helper functions to help create metrics for OTLP
pdata translations. The need for these helpers comes
from PR #5184 in which we need to translate directly
from Prometheus->Pdata instead of Prometheus->OpenCensus->Pdata,
but we shall need to modify both receiver/prometheus*/internal and
the base directory.

This code is heavily used in the mentioned PR and thus no
tests included in this change.

Updates PR #5184

[odeke-em]

Kindly help me with a blind review/approval here @dashpole @alolita @bogdandrutu @Aneurysm9 @codeboten as this code is used in the linked PR.

[codeboten]

👍

[MovieStoreGuy]

To help show the value of this, can I kindly ask you to add tests to this?

[odeke-em]

To help show the value of this, can I kindly ask you to add tests to this?

In the PR subject I explain its purpose and particularly

This code is heavily used in the mentioned PR and thus no
tests included in this change.

[MovieStoreGuy]

If the code is heavily used in the mentioned PR then it should be tested here. Having heavily used code is not an excuse to not write tests.

[bogdandrutu]

Consider to make this internal to the module that needs it for the moment.

[bogdandrutu]

Do not return pointer to pdata.

[bogdandrutu]

pdata usually benefits more if instead of constructing new Metric, you append it to a MetricSlice. Less allocations.

[bogdandrutu]

Also +1 to @MovieStoreGuy comment about having tests.

[odeke-em]

If the code is heavily used in the mentioned PR then it should be tested here. Having heavily used code is not an excuse to not write tests.

I appreciate the enthusiasm and time to review @MovieStoreGuy and @bogdandrutu,. but this doesn't make sense to me, and seems like it is just being pedantic without context about what's happening. This code doesn't add new core code functionality. There is nothing unique about me just copying and pasting this code allover, this isn't code for pdata construction in our libraries, this would be literal helpers in my test code. It costs me much less time to just duplicate this functionality which doesn't add any surface area except in building points effectively. With that logic, all the test helpers should also be tested -- this doesn't hold up for any library, and doesn't add value for the end goals for which these libraries are already years behind in ditching OpenCensus.

[MovieStoreGuy]

@odeke-em , the point of adding tests and smaller PRs is that things can be reviewed in isolation and build up to the larger context as (more bottom up than top down). It helps keeps things streamed line and the community can review with confidence.

This code doesn't add new core code functionality.

From what I can tell with this git commit, it is for the project.

this would be literal helpers in my test code

Aa, I had missed that, my apologies. It wasn't clear these methods were to be only used for tests purposes since the title of the PR and the method names themselves didn't help allude to that fact.

Can I ask that you comment to that affect? Making it clear that these methods are to be used for testing purposes only?

[anuraaga]

With that logic, all the test helpers should also be tested

The reality is this is generally considered good practice - test helpers combine to become a test framework basically. Then we need to catch issues that affect the framework in the framework itself. For example, perhaps some refactoring affects AppendEmpty and breaks this logic - failures showing up with longer stack traces deep in unrelated code will make it very difficult to update this file. In fact, if the same logic was instead copied into five places, at least it's obvious that those five places need to be updated, instead of five unrelated places that you then have to navigate back to here. This is precisely the motivation for unit tests in software engineering.

This code isn't in _test.go files - it's unfortunate that we still haven't reenabled codecov since their security incident since it's great for verifying how we're doing. But the intent is to have a code coverage threshold - if the check was reenabled, this PR wouldn't even be mergable since it wouldn't pass the codecov threshold (applied since it's not _test.go).

They're simple functions and can use just some simple tests so it'd be great if you can add them. I appreciate that it is annoying to write tests - but let's just write them and move on :)

[bogdandrutu]

@anuraaga as suggested:

1. This code should leave in an internal directory for the module that uses it until clear reason to share (even if this is called internalcore it is still shared).
2. The package name can be named "testmetricsutil" or something that suggest to be a testing package.
3. Usually I have test for testing packages as well see https://github.com/open-telemetry/opentelemetry-collector/tree/main/component/componenttest, but I would not enforce that if this is internal only for that module, and marked accordingly as testing package.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[bogdandrutu]

Closing this PR as inactive, since the feedback was not addressed for more than 1m.