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.

Sign up for GitHub

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

[extension/jaegerremotesampling]: Replace Thrift-gen with Proto-gen types for sampling strategies #18485

Merged
merged 24 commits into from
Sep 6, 2023
Merged

[extension/jaegerremotesampling]: Replace Thrift-gen with Proto-gen types for sampling strategies #18485

Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
e24018d
[extension/jaegerremotesampling] move current state into deprecated pkg
frzifus Aug 25, 2023
9bd7d5c
[extension/jaegerremotesampling] remove mod files from deprecated pkg
frzifus Aug 25, 2023
72d7113
[extension/jaegerremotesampling] bump jaeger version from 1.41 to 1.48
frzifus Aug 25, 2023
0d315e7
[extension/jaegerremotesampling] register featuregate
frzifus Aug 25, 2023
5297c36
[pkg/translator/zipkin] bump to jaeger 1.48
frzifus Aug 25, 2023
4e94c5f
[receiver/jaegerreceiver] move current state into deprecated pkg
frzifus Aug 25, 2023
eaf7368
[receiver/jaegerreceiver] bump jaeger to 1.48
frzifus Aug 25, 2023
855a8de
[receiver/jaegerreceiver] register featuregate
frzifus Aug 25, 2023
fb46ce5
*: add changelog entry
frzifus Aug 25, 2023
07d1505
Update extension/jaegerremotesampling/factory.go
frzifus Aug 28, 2023
b990974
jaeger: add otel license header
frzifus Aug 30, 2023
c72a3e7
[extension/jaegerremotesampling] move old jaeger impl into internal
frzifus Aug 30, 2023
e8fe9c1
[receiver/jaegerreceiver] move old jaeger impl into internal
frzifus Aug 30, 2023
e660754
fix pkg paths
frzifus Aug 30, 2023
f80dfe5
*: make gotidy
frzifus Aug 30, 2023
497f7e2
fix
frzifus Aug 30, 2023
3982190
[extension/jaegerremotesampling] fix linting
frzifus Aug 31, 2023
b3045aa
[receiver/jaegerreceiver] fix linting
frzifus Aug 31, 2023
6468cce
Merge branch 'main' into jaegerremotesampling_replace_thrift-gen_with…
frzifus Aug 31, 2023
8e96c1c
Merge branch 'main' into jaegerremotesampling_replace_thrift-gen_with…
frzifus Aug 31, 2023
a4f2962
fix
frzifus Aug 31, 2023
f99ee72
Merge branch 'main' into jaegerremotesampling_replace_thrift-gen_with…
frzifus Aug 31, 2023
c542a2b
Merge branch 'main' into jaegerremotesampling_replace_thrift-gen_with…
frzifus Sep 4, 2023
25bd3b9
Merge branch 'main' into jaegerremotesampling_replace_thrift-gen_with…
frzifus Sep 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
*: add changelog entry 
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
  • Loading branch information
@frzifus
frzifus committed Aug 30, 2023
commit fb46ce53cf8b4a2853b873d852143d8a7a6358a8
19 changes: 19 additions & 0 deletions .chloggen/jaegerremotesampling_replace_thrift-gen_with_proto-gen.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: enhancement

# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
component: jaegerreceiver,jaegerremotesamplingextension
Copy link
Member Author

Choose a reason for hiding this comment

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

After adding a featuregate to the jaegerremotesamplingextension the module worked fine, but it was not possible to compile the contrib client. Since otelcontribcol used 1.48.

Log 
cd ./cmd/otelcontribcol && GO111MODULE=on CGO_ENABLED=0 go build -trimpath -o ../../bin/otelcontribcol_linux_amd64 \
	-tags "" .
# github.com/open-telemetry/opentelemetry-collector-contrib/receiver/jaegerreceiver
../../receiver/jaegerreceiver/trace_receiver.go:174:43: cannot use (*notImplementedConfigManager)(nil) (value of type *notImplementedConfigManager) as configmanager.ClientConfigManager value in variable declaration: *notImplementedConfigManager does not implement configmanager.ClientConfigManager (wrong type for method GetSamplingStrategy)
		have GetSamplingStrategy(context.Context, string) (*"github.com/jaegertracing/jaeger/thrift-gen/sampling".SamplingStrategyResponse, error)
		want GetSamplingStrategy(context.Context, string) (*api_v2.SamplingStrategyResponse, error)
../../receiver/jaegerreceiver/trace_receiver.go:281:74: cannot use &notImplementedConfigManager{} (value of type *notImplementedConfigManager) as configmanager.ClientConfigManager value in argument to httpserver.NewHTTPServer: *notImplementedConfigManager does not implement configmanager.ClientConfigManager (wrong type for method GetSamplingStrategy)
		have GetSamplingStrategy(context.Context, string) (*"github.com/jaegertracing/jaeger/thrift-gen/sampling".SamplingStrategyResponse, error)
		want GetSamplingStrategy(context.Context, string) (*api_v2.SamplingStrategyResponse, error)
# github.com/open-telemetry/opentelemetry-collector-contrib/pkg/translator/zipkin/zipkinv1
../../pkg/translator/zipkin/zipkinv1/thrift.go:27:47: not enough arguments in call to jaegerzipkin.DeserializeThrift
	have ([]byte)
	want (context.Context, []byte)
make: *** [Makefile:287: otelcontribcol] Fehler 1

Therefore, I have increased the version there the same way and provided another feature gate. They work independently of each other.

Copy link
Member Author

Choose a reason for hiding this comment

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

cc @jpkrohling @zcross @MovieStoreGuy

Copy link
Member

Choose a reason for hiding this comment

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

As a temporary solution, it's fine.


# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Add featuregates to replace Thrift-gen with Proto-gen types for sampling strategies

# One or more tracking issues related to the change
issues: [18401]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext: |
Available featuregates are:
- extension.jaegerremotesampling.replaceThriftWithProto
- receiver.jaegerreceiver.replaceThriftWithProto