Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Apr 11, 2022

Conversation

dmitryax
Copy link
Member

@dmitryax dmitryax commented Apr 8, 2022

Split all pdata related code by type and move it from model to the new module pdata.

  • model/pdata and model/otlp are moved to pdata/plog, pdata/pmetric and pdata/ptrace.
  • model/otlpgrpc is moved to pdata/plogotlp, pdata/pmetricotlp and pdata/ptraceotlp.

Now all the APIs in model except for model/semconv are deprecated.

This is another PR following the latest decisions made in #5087 and #5027.

Resolves #4832 and #5027

@dmitryax dmitryax requested review from a team and jpkrohling April 8, 2022 00:12
@dmitryax dmitryax linked an issue Apr 8, 2022 that may be closed by this pull request
@dmitryax dmitryax force-pushed the split-model branch 3 times, most recently from 73c6730 to 0c365e7 Compare April 8, 2022 01:21
@codecov
Copy link

codecov bot commented Apr 8, 2022

Codecov Report

Merging #5168 (9effe2d) into main (2a55e9f) will decrease coverage by 35.45%.
The diff coverage is 80.36%.

❗ Current head 9effe2d differs from pull request most recent head 1874760. Consider uploading reports for the commit 1874760 to get more accurate results

@@             Coverage Diff             @@
##             main    #5168       +/-   ##
===========================================
- Coverage   90.32%   54.87%   -35.46%     
===========================================
  Files         185      197       +12     
  Lines       11042    23379    +12337     
===========================================
+ Hits         9974    12829     +2855     
- Misses        841     9615     +8774     
- Partials      227      935      +708     
Impacted Files Coverage Δ
component/receiver.go 100.00% <ø> (ø)
pdata/internal/common.go 94.94% <ø> (ø)
pdata/internal/data/bytesid.go 81.25% <ø> (ø)
...data/protogen/collector/logs/v1/logs_service.pb.go 30.68% <ø> (ø)
...rotogen/collector/metrics/v1/metrics_service.pb.go 30.68% <ø> (ø)
...ata/protogen/collector/trace/v1/trace_config.pb.go 0.87% <ø> (ø)
...ta/protogen/collector/trace/v1/trace_service.pb.go 30.68% <ø> (ø)
...data/internal/data/protogen/common/v1/common.pb.go 18.89% <ø> (ø)
pdata/internal/data/protogen/logs/v1/logs.pb.go 32.71% <ø> (ø)
...ta/internal/data/protogen/metrics/v1/metrics.pb.go 17.10% <ø> (ø)
... and 79 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 2a55e9f...1874760. Read the comment docs.

@jpkrohling
Copy link
Member

I'm requesting the review from the same reviewers of the original PR.

@tigrannajaryan
Copy link
Member

2 questions:

  • pdata is now one Go module. I thought we planned to split into 4 modules, one common and 3 for signals types. Was this decision changed or it is coming in a future PR?
  • What is the plan for model/semconv? Is model going to remain a Go module with just semconv package?

@dmitryax
Copy link
Member Author

dmitryax commented Apr 8, 2022

@tigrannajaryan

pdata is now one Go module. I thought we planned to split into 4 modules, one common and 3 for signals types. Was this decision changed or it is coming in a future PR?

The plan is the same. New modules for each signal + pdata/pcommon will be added later. They depend on pdata/internal, so we need to make pdata or pdata/internal a module anyway. I think we can keep additional pdata module for that.

What is the plan for model/semconv? Is model going to remain a Go module with just semconv package?

I think we agreed on moving it from /model/semconv to another separate /semconv module. This will be done separately.

pdata/internal/semconv/template.j2 Outdated Show resolved Hide resolved
@dmitryax dmitryax marked this pull request as draft April 8, 2022 17:49
@dmitryax dmitryax force-pushed the split-model branch 2 times, most recently from 424c863 to e56b0f4 Compare April 8, 2022 18:56
@dmitryax dmitryax marked this pull request as ready for review April 8, 2022 18:58
@dmitryax
Copy link
Member Author

dmitryax commented Apr 8, 2022

Look like something wrong with coverage calculation. I verified that the coverage is the same between the moved parts

@bogdandrutu
Copy link
Member

@dmitryax
Copy link
Member Author

@dmitryax look into updating this file https://github.com/open-telemetry/opentelemetry-collector/blob/main/.codecov.yml

Oh that's why! Thanks for pointing on this

pdata/ptrace/alias.go Outdated Show resolved Hide resolved
pdata/plog/json.go Outdated Show resolved Hide resolved
pdata/plog/pb.go Outdated Show resolved Hide resolved
Split all `pdata` related code by type and move it from `model` to the new module `pdata`.

- `model/pdata` and `model/otlp` are moved to `pdata/plog`, `pdata/pmetric` and `pdata/ptrace`.
- `model/otlpgrpc` is moved to `pdata/plogotlp`, `pdata/pmetricotlp` and `pdata/ptraceotlp`.

Now all the API in `model` except for `model/semconv` is deprecated.
@bogdandrutu bogdandrutu merged commit 734e25e into open-telemetry:main Apr 11, 2022
@dmitryax
Copy link
Member Author

🎉

@dmitryax dmitryax deleted the split-model branch April 12, 2022 00:32
sumo-drosiek pushed a commit to SumoLogic/sumologic-otel-collector that referenced this pull request Apr 19, 2022
swiatekm pushed a commit to SumoLogic/sumologic-otel-collector that referenced this pull request Apr 25, 2022
swiatekm pushed a commit to SumoLogic/sumologic-otel-collector that referenced this pull request Apr 25, 2022
swiatekm pushed a commit to SumoLogic/sumologic-otel-collector that referenced this pull request Apr 25, 2022
kasia-kujawa pushed a commit to SumoLogic/sumologic-otel-collector that referenced this pull request May 13, 2022
Nicholaswang pushed a commit to Nicholaswang/opentelemetry-collector that referenced this pull request Jun 7, 2022
…pen-telemetry#5168)

Split all `pdata` related code by type and move it from `model` to the new module `pdata`.

- `model/pdata` and `model/otlp` are moved to `pdata/plog`, `pdata/pmetric` and `pdata/ptrace`.
- `model/otlpgrpc` is moved to `pdata/plogotlp`, `pdata/pmetricotlp` and `pdata/ptraceotlp`.

Now all the API in `model` except for `model/semconv` is deprecated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split otlp/otlpgrpc Split pdata into multiple packages
5 participants