Split pdata package by telemetry signal type #4918

dmitryax · 2022-02-25T00:19:58Z

This is the next change after #4833 following the approach outlined as Option 3 in this issue. It adds new aliases for pdata split into different packages by signal type. The following package structure is proposed:

/model
  /pdata (aliases to /internal/pdata)
    /traces (additional aliases to traces parts of internal/pdata)
    /logs (additional aliases to logs parts of internal/pdata)
    /metrics (additional aliases to metrics parts of internal/pdata)

Next steps:

Remove traces, logs and metrics parts from /model/pdata.
Move traces, logs and metrics implementation from /model/internal/pdata to /model/pdata/<signals> to replace the aliases - not a breaking change.

Once all the steps are done the following package structure will be achieved:

/model
  /pdata (aliases to /internal/pdata where only common parts are left)
    /traces (actual implementation of traces related pdata parts)
    /logs (actual implementation of logs related pdata parts)
    /metrics (actual implementation of metrics related pdata parts)

codecov · 2022-02-25T00:22:55Z

Codecov Report

Merging #4918 (e52bb60) into main (0f07b0b) will increase coverage by 0.03%.
The diff coverage is 98.11%.

@@            Coverage Diff             @@
##             main    #4918      +/-   ##
==========================================
+ Coverage   89.93%   89.96%   +0.03%     
==========================================
  Files         183      183              
  Lines       11104    11104              
==========================================
+ Hits         9986     9990       +4     
+ Misses        892      889       -3     
+ Partials      226      225       -1

Impacted Files	Coverage Δ
component/receiver.go	`100.00% <ø> (ø)`
model/internal/pdata/generated_logs.go	`96.65% <ø> (ø)`
model/internal/pdata/generated_traces.go	`96.89% <ø> (ø)`
model/internal/pdata/logs.go	`81.48% <ø> (ø)`
model/internal/pdata/traces.go	`77.77% <ø> (ø)`
...ernal/internalconsumertest/err_or_sink_consumer.go	`60.60% <50.00%> (ø)`
model/otlp/json_unmarshaler.go	`79.31% <66.66%> (ø)`
consumer/consumererror/signalerrors.go	`100.00% <100.00%> (ø)`
consumer/consumertest/err.go	`100.00% <100.00%> (ø)`
consumer/consumertest/nop.go	`100.00% <100.00%> (ø)`
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f07b0b...e52bb60. Read the comment docs.

bogdandrutu

LGTM this is how I would envisioned to continue that PR.

tigrannajaryan · 2022-02-28T15:09:44Z

LGTM to me as well. Only small question: do we want common aliases to be in model/pdata or model/pdata/common?

dmitryax · 2022-03-01T07:56:35Z

Only small question: do we want common aliases to be in model/pdata or model/pdata/common?

This is a good question.

model/pdata pros:

I think names look more descriptive, e.g. pdata.InstrumentationLibrary vs common.InstrumentationLibrary.
No need to introduce another breaking change.

model/pdata/common pros:

Maybe more cleaner packages/modules structure when all the signals+common are on the same level.

Maybe I missed something else?

I'd prefer model/pdata, but not firm on this.

tigrannajaryan · 2022-03-01T14:21:01Z

I'd prefer model/pdata, but not firm on this.

Works for me.

model/pdata/traces/alias_trace.go

Aneurysm9 · 2022-03-03T01:03:15Z

If we are going to split other components, they can be changed along with switching to the new <signals> packages.
If we don't split them, we will need to run all the components relying on pdata through the 3-step deprecation process.

Can you elaborate on this? I'm not sure I follow.

Aneurysm9 · 2022-03-03T01:09:42Z

If we choose this route, should we jump straight to this PR and skip #4833? I'm not seeing the benefit of it as an intermediate step. Is there something I'm missing there?

dmitryax · 2022-03-03T21:52:56Z

If we are going to split other components, they can be changed along with switching to the new packages.
If we don't split them, we will need to run all the components relying on pdata through the 3-step deprecation process.

Can you elaborate on this? I'm not sure I follow.

I tried to outline the next steps towards moving from unified pdata to the new separate packages. To do that we need to update all the clients of pdata. There is no yet decision whether we will be splitting all other packages by the signal type. So there are two possible ways:

Split all the client packages and update them to use new pdata/signal packages,
Otherwise we need to run the clients through the 2-step (not 3) deprecation process as outlined here

dmitryax · 2022-03-03T22:00:44Z

If we choose this route, should we jump straight to this PR and skip #4833? I'm not seeing the benefit of it as an intermediate step. Is there something I'm missing there?

I would prefer granular changes as they are pretty much independent steps. Just to keep clean git history with smaller change sets

alolita · 2022-03-04T19:26:57Z

For the sake of user clarity, we should support intuitive conventions

Example -
/pdata/traces
/pdata/metrics
/pdata/logs

which is what this PR proposed and is implementing. So +1 on this PR.

bogdandrutu · 2022-03-04T22:03:09Z

@alolita thanks for the feedback. I think the current PR I think we use different package names. @dmitryax would be good to see both versions to make a decision, not sure which one I prefer. Maybe we can just write some fake usage examples to see how they will look

github-actions · 2022-03-19T03:15:52Z

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

dmitryax · 2022-03-25T08:06:04Z

@bogdandrutu

I added a small usage example of the new pdata packages as further split of consumer package: #5086

I also added a PR with the alternative package structure and the same usage example: #5087

I think I like structure from this PR more because:

The package names are probably better aligned with names of other packages that we will be splitting going forward even there is a possible chance of import conflict.
At least some API names can be shorten like plog.LogsMarshaler -> logs.Marshaler, plog.NewLogs = logs.New.

Let me know WDYT. I will also update more packages to show usage of both version more extensively.

bogdandrutu · 2022-03-25T09:02:16Z

The package names are probably better aligned with names of other packages that we will be splitting going forward even there is a possible chance of import conflict.

As mentioned in the other PRs, I think we agreed to not do this. Also if from the same repo we export conflicting packages I think we fail a bit the language pattern :)

At least some API names can be shorten like plog.LogsMarshaler -> logs.Marshaler, plog.NewLogs = logs.New.

This is nice indeed.

dmitryax · 2022-03-28T04:59:27Z

Also if from the same repo we export conflicting packages I think we fail a bit the language pattern :)

I would agree on this one. Is this the only reason why you prefer #5087 over this PR?

Aneurysm9 · 2022-03-28T16:39:32Z

Also if from the same repo we export conflicting packages I think we fail a bit the language pattern :)

I would agree on this one. Is this the only reason why you prefer #5087 over this PR?

I disagree. I think it is less about whether there are multiple packages with the same package name within the repository and more about whether they are likely to be imported at the same time. What other packages would conflict with these names?

dmitryax · 2022-03-28T22:47:28Z

What other packages would conflict with these names?

It depends on whether we will be splitting other packages. If we split them, it'll be at least component and consumer packages. Currently we don't have anything conflicting.

dmitryax · 2022-03-29T23:23:19Z

As I described here, it's not required to change any clients to split the pdata package. We can split it right away.

The only thing we need to decide is which package structure we are going to adopt. I updated all the core's code in this PR and #5087 to use new packages and deprecated the old one.

There are few conflicts with existing metrics, traces and logs local variables in this PR. Now I'm on a fence and maybe prefer #5087 slightly more because plog, ptrace, pmetric and pcommon are pretty unique names and won't cause any conflicts. I'm going to suggest a poll so we can vote and decide on tomorrow's SIG meeting.

cc @Aneurysm9 @bogdandrutu @tigrannajaryan

dmitryax · 2022-03-30T18:13:27Z

Results of the poll from today's SIG meeting https://docs.google.com/document/d/1D_jK39GYkp9wAPgRoCYntnzq4LBs_YNnu64vXJGc9nI/edit#heading=h.oeev18el9qk1:

2 votes for this structure.
7 votes for the alternative structure.

So I'm closing this PR in favor of #5087

dmitryax requested a review from a team as a code owner February 25, 2022 00:19

dmitryax requested a review from tigrannajaryan February 25, 2022 00:19

bogdandrutu reviewed Feb 25, 2022

View reviewed changes

dmitryax marked this pull request as draft February 25, 2022 01:36

dmitryax mentioned this pull request Feb 25, 2022

Split pdata into multiple packages #4832

Closed

bogdandrutu reviewed Mar 1, 2022

View reviewed changes

model/pdata/traces/alias_trace.go Outdated Show resolved Hide resolved

dmitryax mentioned this pull request Mar 2, 2022

Move pdata into an internal package and use aliases in exported package #4833

Merged

dmitryax force-pushed the pdata-signal-aliases branch from ae0b1a6 to 7117b3c Compare March 2, 2022 02:33

Aneurysm9 mentioned this pull request Mar 2, 2022

Split pdata package without using aliases #4919

Closed

dmitryax force-pushed the pdata-signal-aliases branch from 7117b3c to 9b87abc Compare March 4, 2022 07:17

Aneurysm9 added this to the `go.opentelemetry.io/collector/model` 1.0 milestone Mar 4, 2022

github-actions bot added the Stale label Mar 19, 2022

dmitryax removed the Stale label Mar 19, 2022

dmitryax force-pushed the pdata-signal-aliases branch 2 times, most recently from 971a734 to 6a68c81 Compare March 25, 2022 06:31

This was referenced Mar 25, 2022

Introduce additional consumer packages split by telemetry type #5086

Closed

Split pdata package by telemetry signal type #5087

Closed

dmitryax force-pushed the pdata-signal-aliases branch 2 times, most recently from c71caf5 to fd63b56 Compare March 28, 2022 22:42

dmitryax force-pushed the pdata-signal-aliases branch 2 times, most recently from b59c1ab to 321f935 Compare March 29, 2022 18:47

dmitryax changed the title ~~Add pdata aliases separated by signals~~ Split pdata aliases separated by signals Mar 29, 2022

dmitryax changed the title ~~Split pdata aliases separated by signals~~ Split pdata package by telemetry signal type Mar 29, 2022

dmitryax force-pushed the pdata-signal-aliases branch from 321f935 to ab7bf1f Compare March 30, 2022 04:43

dmitryax added 4 commits March 29, 2022 22:09

Create additional pdata packages separated by type for further split

6b174c3

Adopt usage of split pdata in core

ced4685

Deprecate usage of the old pdata API

e289bc9

Remove other deprecated usages

e52bb60

dmitryax force-pushed the pdata-signal-aliases branch from ab7bf1f to e52bb60 Compare March 30, 2022 05:09

dmitryax closed this Mar 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split pdata package by telemetry signal type #4918

Split pdata package by telemetry signal type #4918

dmitryax commented Feb 25, 2022 •

edited

codecov bot commented Feb 25, 2022 •

edited

bogdandrutu left a comment

tigrannajaryan commented Feb 28, 2022

dmitryax commented Mar 1, 2022 •

edited

tigrannajaryan commented Mar 1, 2022

Aneurysm9 commented Mar 3, 2022

Aneurysm9 commented Mar 3, 2022

dmitryax commented Mar 3, 2022 •

edited

dmitryax commented Mar 3, 2022 •

edited

alolita commented Mar 4, 2022

bogdandrutu commented Mar 4, 2022 •

edited

github-actions bot commented Mar 19, 2022

dmitryax commented Mar 25, 2022 •

edited

bogdandrutu commented Mar 25, 2022

dmitryax commented Mar 28, 2022

Aneurysm9 commented Mar 28, 2022

dmitryax commented Mar 28, 2022

dmitryax commented Mar 29, 2022

dmitryax commented Mar 30, 2022

Split pdata package by telemetry signal type #4918

Split pdata package by telemetry signal type #4918

Conversation

dmitryax commented Feb 25, 2022 • edited

codecov bot commented Feb 25, 2022 • edited

Codecov Report

bogdandrutu left a comment

Choose a reason for hiding this comment

tigrannajaryan commented Feb 28, 2022

dmitryax commented Mar 1, 2022 • edited

tigrannajaryan commented Mar 1, 2022

Aneurysm9 commented Mar 3, 2022

Aneurysm9 commented Mar 3, 2022

dmitryax commented Mar 3, 2022 • edited

dmitryax commented Mar 3, 2022 • edited

alolita commented Mar 4, 2022

bogdandrutu commented Mar 4, 2022 • edited

github-actions bot commented Mar 19, 2022

dmitryax commented Mar 25, 2022 • edited

bogdandrutu commented Mar 25, 2022

dmitryax commented Mar 28, 2022

Aneurysm9 commented Mar 28, 2022

dmitryax commented Mar 28, 2022

dmitryax commented Mar 29, 2022

dmitryax commented Mar 30, 2022

dmitryax commented Feb 25, 2022 •

edited

codecov bot commented Feb 25, 2022 •

edited

dmitryax commented Mar 1, 2022 •

edited

dmitryax commented Mar 3, 2022 •

edited

dmitryax commented Mar 3, 2022 •

edited

bogdandrutu commented Mar 4, 2022 •

edited

dmitryax commented Mar 25, 2022 •

edited