Split pdata package by telemetry signal type #5087

dmitryax · 2022-03-25T07:24:02Z

This is an alternative to #4918 with the following package structure:

/model
  /pcommon (aliases to common API in /internal/pdata: Resource, Map, Value etc)
  /ptrace (aliases to traces API of internal/pdata)
  /plog (aliases to logs API of internal/pdata)
  /pmetric (aliases to metrics API of internal/pdata)

codecov · 2022-03-25T07:27:39Z

Codecov Report

Merging #5087 (f28481b) into main (caa4f27) will decrease coverage by 0.19%.
The diff coverage is 100.00%.

❗ Current head f28481b differs from pull request most recent head 762e945. Consider uploading reports for the commit 762e945 to get more accurate results

@@            Coverage Diff             @@
##             main    #5087      +/-   ##
==========================================
- Coverage   90.34%   90.14%   -0.20%     
==========================================
  Files         182      183       +1     
  Lines       11031    11022       -9     
==========================================
- Hits         9966     9936      -30     
- Misses        840      864      +24     
+ Partials      225      222       -3

Impacted Files	Coverage Δ
model/internal/pdata/generated_plog.go	`96.65% <ø> (ø)`
model/internal/pdata/generated_pmetric.go	`97.00% <ø> (ø)`
model/internal/pdata/generated_ptrace.go	`96.89% <ø> (ø)`
consumer/internal/consumer.go	`100.00% <100.00%> (ø)`
model/otlpgrpc/metrics.go	`43.54% <0.00%> (-20.39%)`	⬇️
model/otlpgrpc/logs.go	`43.54% <0.00%> (-16.46%)`	⬇️
model/otlpgrpc/traces.go	`43.54% <0.00%> (-16.46%)`	⬇️
model/otlp/json_unmarshaler.go	`76.92% <0.00%> (-2.39%)`	⬇️
config/mapprovider/filemapprovider/mapprovider.go	`82.35% <0.00%> (-1.86%)`	⬇️
config/configgrpc/configgrpc.go	`92.68% <0.00%> (-0.25%)`	⬇️
... and 14 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 caa4f27...762e945. Read the comment docs.

bogdandrutu

I like this, except the split of consumer which is out of scope for the pdata split.

dmitryax · 2022-03-28T04:24:42Z

Removed the split of consumer

dmitryax · 2022-03-30T18:15:09Z

Based on the results of the poll (7:2), #4918 is closed in favor of this PR

bogdandrutu · 2022-04-01T14:58:09Z

@open-telemetry/collector-maintainers please review

codeboten

Overall this looks good, just a couple of comments

CHANGELOG.md

exporter/exporterhelper/internal/persistent_queue_test.go

tigrannajaryan

LGTM.

I think in our discussions we can continue referring to the internal data collectively as pdata even when it is totally eliminated from the codebase. It is a convenient and catchy name.

dmitryax · 2022-04-04T17:51:24Z

I think in our discussions we can continue referring to the internal data collectively as pdata even when it is totally eliminated from the codebase. It is a convenient and catchy name.

I agree on using pdata name as reference to the all pipeline data structures. We agreed on using p<signal> as module names, but we still can to move them under an extra pdata directory and have the following structure if we want to:

/model
  /pdata
    /pcommon (aliases to common API in /internal/pdata: Resource, Map, Value etc)
    /ptrace (aliases to traces API of internal/pdata)
    /plog (aliases to logs API of internal/pdata)
    /pmetric (aliases to metrics API of internal/pdata)
  /semconv
  ...

@open-telemetry/collector-approvers please share you feedback if you think it'll be better.

tigrannajaryan · 2022-04-04T18:07:04Z

We agreed on using p<signal> as module names, but we still can to move them under an extra pdata directory

This approach works for me too. No strong opinion one way or another.

bogdandrutu · 2022-04-04T18:12:32Z

@dmitryax @tigrannajaryan what about a model -> pdata than semconv is it's own thing outside pdata module.

dmitryax · 2022-04-04T18:16:23Z

what about a model -> pdata than semconv is it's own thing outside pdata module.

I was also thinking about it. It means that we need to move model/otlp and model/otlpgrpc to pdata/p<signal>, right? I can submit a followup PR to see how it looks.

tigrannajaryan · 2022-04-04T18:16:43Z

@dmitryax @tigrannajaryan what about a model -> pdata than semconv is it's own thing outside pdata module.

We have other packages in model which strictly speaking are not pdata unless we stretch the term (which I would prefer not to). pdata in my mind is in-memory representation of Collector pipeline data.

bogdandrutu · 2022-04-04T18:19:03Z

I was also thinking about it. It means that we need to move model/otlp and model/otlpgrpc to pdata/p, right? I can submit a followup PR to see how it looks.

See #5027

We have other packages in model which strictly speaking are not pdata unless we stretch the term (which I would prefer not to). pdata in my mind is in-memory representation of Collector pipeline data.

Besides semconv nothing after we fix #5027

Aneurysm9 · 2022-04-04T19:03:47Z

We have other packages in model which strictly speaking are not pdata unless we stretch the term (which I would prefer not to). pdata in my mind is in-memory representation of Collector pipeline data.

This is how I think of it as well. It seems sufficiently different from OTLP, even if it is presently implemented as an API for managing an OTLP representation, that I think we should be careful about intermixing the concepts too much.

dmitryax · 2022-04-06T18:10:40Z

Looks like we have an agreement on how to merge otlp and otlpgrpc packages into pdata in #5027. According to that we will have the following modules instead if /model:

/pdata
  /ptrace (module)
    /ptraceotlp
  /pmetric (module)
    /pmetricotlp
  /plog (module)
    /plogotlp

/semconv (module)

If everyone is ok with this structure, I'm going to proceed with the following action items:

Move /model/internal to /internal
Update this PR to have /pdata/p<signal> instead of /model/p<signal>
Address Split otlp/otlpgrpc #5027 by introducing /model/p<signal>/p<signal>oltp packages

cc @bogdandrutu @tigrannajaryan @Aneurysm9

dmitryax · 2022-04-06T18:52:19Z

I tried a RP for (1) item #5161. It works but introduces interdependencies between go.opentelemetry.io/collector and go.opentelemetry.io/collector/model modules. So I'd rather update this PR in a different way to avoid the interdependencies but use more aliasing for the deprecated APIs.

dmitryax · 2022-04-08T00:18:33Z

It appeared to be pretty difficult to do what we agreed on by the steps that I outlined without introducing interdependencies between modules. To make it in a clean way, splitting of all the pdata related parts into a new module has to be done in one shot. I submitted another PR for that instead of reusing this one: #5168

dmitryax requested a review from a team as a code owner March 25, 2022 07:24

dmitryax requested a review from jpkrohling March 25, 2022 07:24

dmitryax force-pushed the split-pdata-alternative branch from 58f49f9 to 9d76f9a Compare March 25, 2022 07:26

dmitryax force-pushed the split-pdata-alternative branch from 9d76f9a to 00bf3e9 Compare March 25, 2022 07:29

dmitryax marked this pull request as draft March 25, 2022 07:42

dmitryax force-pushed the split-pdata-alternative branch 2 times, most recently from 449d4b5 to c8471e8 Compare March 25, 2022 08:04

dmitryax mentioned this pull request Mar 25, 2022

Split pdata package by telemetry signal type #4918

Closed

dmitryax force-pushed the split-pdata-alternative branch from c8471e8 to f28481b Compare March 25, 2022 08:17

bogdandrutu requested changes Mar 25, 2022

View reviewed changes

dmitryax force-pushed the split-pdata-alternative branch from f28481b to 4f48b62 Compare March 28, 2022 04:24

dmitryax mentioned this pull request Mar 28, 2022

Introduce additional consumer packages split by telemetry type #5086

Closed

dmitryax force-pushed the split-pdata-alternative branch 4 times, most recently from 9230a4f to 4b2a570 Compare March 29, 2022 20:44

dmitryax changed the title ~~Create additional pdata packages separated by signal type (alternative structure)~~ Split pdata package by telemetry signal type (alternative structure) Mar 29, 2022

dmitryax force-pushed the split-pdata-alternative branch 3 times, most recently from e153289 to 7b6470e Compare March 29, 2022 22:56

dmitryax marked this pull request as ready for review March 30, 2022 18:13

dmitryax changed the title ~~Split pdata package by telemetry signal type (alternative structure)~~ Split pdata package by telemetry signal type Mar 30, 2022

dmitryax force-pushed the split-pdata-alternative branch from 7b6470e to c8df699 Compare March 30, 2022 18:20

bogdandrutu approved these changes Apr 1, 2022

View reviewed changes

codeboten reviewed Apr 1, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

exporter/exporterhelper/internal/persistent_queue_test.go Show resolved Hide resolved

dmitryax force-pushed the split-pdata-alternative branch from c8df699 to bfb7a9c Compare April 1, 2022 17:29

codeboten approved these changes Apr 1, 2022

View reviewed changes

dmitryax mentioned this pull request Apr 1, 2022

[pdata] Consider removing wrapping for Span/TraceID #5137

Closed

dmitryax force-pushed the split-pdata-alternative branch 2 times, most recently from 07f23e2 to e1f15ac Compare April 1, 2022 21:59

tigrannajaryan approved these changes Apr 4, 2022

View reviewed changes

Split pdata package by telemetry signal type

762e945

dmitryax force-pushed the split-pdata-alternative branch from e1f15ac to 762e945 Compare April 4, 2022 23:46

dmitryax mentioned this pull request Apr 5, 2022

Split otlp/otlpgrpc #5027

Closed

dmitryax marked this pull request as draft April 6, 2022 18:10

This was referenced Apr 6, 2022

[pdata] Extract model/internal/pdata package into model/internal #5162

Merged

Make InstrumentationLibrary<signal>ToScope functions unexported #5164

Merged

Split model by signal type and move it to the new pdata module #5168

Merged

bogdandrutu closed this Apr 11, 2022

dmitryax deleted the split-pdata-alternative branch April 12, 2022 00:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split pdata package by telemetry signal type #5087

Split pdata package by telemetry signal type #5087

dmitryax commented Mar 25, 2022 •

edited

Loading

codecov bot commented Mar 25, 2022 •

edited

Loading

bogdandrutu left a comment

dmitryax commented Mar 28, 2022

dmitryax commented Mar 30, 2022 •

edited

Loading

bogdandrutu commented Apr 1, 2022 •

edited

Loading

codeboten left a comment

tigrannajaryan left a comment

dmitryax commented Apr 4, 2022

tigrannajaryan commented Apr 4, 2022

bogdandrutu commented Apr 4, 2022

dmitryax commented Apr 4, 2022

tigrannajaryan commented Apr 4, 2022

bogdandrutu commented Apr 4, 2022

Aneurysm9 commented Apr 4, 2022

dmitryax commented Apr 6, 2022 •

edited

Loading

dmitryax commented Apr 6, 2022 •

edited

Loading

dmitryax commented Apr 8, 2022 •

edited

Loading

Split pdata package by telemetry signal type #5087

Split pdata package by telemetry signal type #5087

Conversation

dmitryax commented Mar 25, 2022 • edited Loading

codecov bot commented Mar 25, 2022 • edited Loading

Codecov Report

bogdandrutu left a comment

Choose a reason for hiding this comment

dmitryax commented Mar 28, 2022

dmitryax commented Mar 30, 2022 • edited Loading

bogdandrutu commented Apr 1, 2022 • edited Loading

codeboten left a comment

Choose a reason for hiding this comment

tigrannajaryan left a comment

Choose a reason for hiding this comment

dmitryax commented Apr 4, 2022

tigrannajaryan commented Apr 4, 2022

bogdandrutu commented Apr 4, 2022

dmitryax commented Apr 4, 2022

tigrannajaryan commented Apr 4, 2022

bogdandrutu commented Apr 4, 2022

Aneurysm9 commented Apr 4, 2022

dmitryax commented Apr 6, 2022 • edited Loading

dmitryax commented Apr 6, 2022 • edited Loading

dmitryax commented Apr 8, 2022 • edited Loading

dmitryax commented Mar 25, 2022 •

edited

Loading

codecov bot commented Mar 25, 2022 •

edited

Loading

dmitryax commented Mar 30, 2022 •

edited

Loading

bogdandrutu commented Apr 1, 2022 •

edited

Loading

dmitryax commented Apr 6, 2022 •

edited

Loading

dmitryax commented Apr 6, 2022 •

edited

Loading

dmitryax commented Apr 8, 2022 •

edited

Loading