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

Separate /model into its own module #5144

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

chirag-ghosh
Copy link
Contributor

Which problem is this PR solving?

Description of the changes

  • Created go module at /model. Used replace in go.mod at / and /model
  • Updated /scripts/check-go-version.sh to check /model/go.mod as well.
  • Added support in dependabot (opentelemetry moved from dependabot to renovate-bot here. We can use dependabot for multiple modules though.)
  • For releases, we can use multimod by opentelemetry. A versions.yaml will define the different versions of different modules. (I have not implemented this yet.)
  • Code coverage should not be affected I feel. Although let's see the codecov report.

How was this change tested?

  • make lint test

Checklist

Signed-off-by: Chirag Ghosh <cghosh828049@gmail.com>
Signed-off-by: Chirag Ghosh <cghosh828049@gmail.com>
Signed-off-by: Chirag Ghosh <cghosh828049@gmail.com>
Signed-off-by: Chirag Ghosh <cghosh828049@gmail.com>
@chirag-ghosh chirag-ghosh requested a review from a team as a code owner January 25, 2024 14:44
require (
github.com/apache/thrift v0.19.0
github.com/gogo/protobuf v1.3.2
github.com/jaegertracing/jaeger v0.0.0-00010101000000-000000000000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean we have circular dependency? Because in that case the whole exercise is pointless, the whole reason for separating model was so that OTEL contrib could import it with the rest of jaeger deps. I think model should have very minimal deps, we can probably separate them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we can remove this. this was automatically added when i ran go mod tidy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only dependency of jaeger in /model is in tests like here. I think we need to have the testutils inside /model as well to remove the dependency.

Copy link
Contributor

@mmorel-35 mmorel-35 Jan 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see model/adjuster/package_test.go
Pointing to pkg/utils, which can explain the circular dependency.
There might be more that this one

Edit. There is also an import from thrift-gen and proto-gen packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to remove the circular dependency we will need to either copy the same things inside /model or make them separate packages as well

@@ -22,6 +22,7 @@ require (
github.com/grpc-ecosystem/go-grpc-middleware v1.4.0
github.com/hashicorp/go-hclog v1.6.2
github.com/hashicorp/go-plugin v1.6.0
github.com/jaegertracing/jaeger/model v0.0.0-00010101000000-000000000000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the significance of this weird version? Is dependabot going to try to change it? Did you try on a separate repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try in my repository and see what happens

Copy link

codecov bot commented Jan 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8183d12) 95.62% compared to head (7dd56dd) 95.34%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5144      +/-   ##
==========================================
- Coverage   95.62%   95.34%   -0.28%     
==========================================
  Files         324      324              
  Lines       18590    18590              
==========================================
- Hits        17776    17725      -51     
- Misses        653      707      +54     
+ Partials      161      158       -3     
Flag Coverage Δ
cassandra-3.x 25.58% <ø> (ø)
cassandra-4.x 25.58% <ø> (ø)
elasticsearch-5.x 19.86% <ø> (ø)
elasticsearch-6.x 19.86% <ø> (ø)
elasticsearch-7.x 19.98% <ø> (-0.02%) ⬇️
elasticsearch-8.x 20.08% <ø> (ø)
grpc-badger 19.48% <ø> (-0.02%) ⬇️
kafka 3.92% <ø> (-10.17%) ⬇️
opensearch-1.x 20.00% <ø> (ø)
opensearch-2.x 19.98% <ø> (-0.02%) ⬇️
unittests 93.32% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@chirag-ghosh
Copy link
Contributor Author

@yurishkuro Can you please help me figure out why the code coverage went down?

@yurishkuro
Copy link
Member

Can you please help me figure out why the code coverage went down?

In the report I see that all /model/ files are down to 0% coverage. I suspect go test ./... does not include sub-modules - this is one of the things that needs to be figured out.

@yurishkuro yurishkuro added the changelog:new-feature Change that should be called out as new feature in CHANGELOG label Jan 26, 2024
@yurishkuro
Copy link
Member

another issue - the vulnerability checker flagged /model/ as vulnerable dependency, I suspect because of 0.x version number. What is the story with this version number? In OTEL collector I see that their versions are in sync across modules, so they must have some mechanism for managing/updating those after release.

Vulnerabilities
  model/go.mod » github.com/jaegertracing/jaeger@0.0.0-00010101000000-000000000000 – A stored XSS in jaeger UI might allow an attacker who controls a trace to perform arbitrary jaeger queries (moderate severity)
    ↪ https://github.com/advisories/GHSA-2w[8](https://github.com/jaegertracing/jaeger/actions/runs/7656173413/job/20863882760?pr=5144#step:5:9)w-qhg4-f78j
  model/go.mod » github.com/jaegertracing/jaeger@0.0.0-00010101000000-000000000000 – Information Exposure in jaeger (moderate severity)
    ↪ https://github.com/advisories/GHSA-gh32-pc56-4c[9](https://github.com/jaegertracing/jaeger/actions/runs/7656173413/job/20863882760?pr=5144#step:5:10)6
  Error: Dependency review detected vulnerable packages.

Signed-off-by: Chirag Ghosh <cghosh828049@gmail.com>
@chirag-ghosh
Copy link
Contributor Author

The wierd versioning happended after I aded the replace lines and ran go mod tidy. Here is a StackOverflow question regarding it.

Signed-off-by: Chirag Ghosh <cghosh828049@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:new-feature Change that should be called out as new feature in CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Separate /model into its own module
3 participants