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

deps: Remove opentracing #27246

Closed
wants to merge 1 commit into from
Closed

Conversation

phlax
Copy link
Member

@phlax phlax commented May 7, 2023

As explained here #9958 (comment) the opentracing build is currently broken due to cmake being removed from the host environment, and is due to be removed.

This should have been scheduled for deprecation a long time ago as there was a public conversation and some shared expectation among dep-shepherds that this would be removed

We can postpone this landing until after the next release if we can find a solution to #27244, but currently the build is broken (eg for anyone building locally without an existing cache) so probably the better thing is to land this and revert temporarily if someone provides an adequate fix

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@phlax phlax requested a review from wbpcode as a code owner May 7, 2023 20:57
@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label May 7, 2023
@phlax phlax marked this pull request as draft May 7, 2023 20:57
@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @moderation

🐱

Caused by: #27246 was opened by phlax.

see: more, trace.

@phlax phlax force-pushed the remove-opentracing branch 2 times, most recently from e7ff68c to 4753f0f Compare May 7, 2023 22:10
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #27246 was synchronize by phlax.

see: more, trace.

@phlax phlax changed the title [TESTING/WIP] deps: Remove opentracing [WIP] deps: Remove opentracing May 7, 2023
@phlax
Copy link
Member Author

phlax commented May 7, 2023

seems the only thing that is depending on the opentracing lib is jaeger-native example for its config

this example uses a binary of uncertain origin so has always been problematic, and not overly helpful as an example

im wondering if we want to keep it and fix the config, or just remove this also (mho is probably the latter)

@phlax phlax changed the title [WIP] deps: Remove opentracing deps: Remove opentracing May 7, 2023
@phlax phlax marked this pull request as ready for review May 7, 2023 22:15
Signed-off-by: Ryan Northey <[email protected]>
@wbpcode
Copy link
Member

wbpcode commented May 8, 2023

/watch

@phlax
Copy link
Member Author

phlax commented May 8, 2023

/wait on outcome of conversations in #9958 and/or #27244

@moderation
Copy link
Contributor

/lgtm deps

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label May 8, 2023
@Oberon00
Copy link
Contributor

So from this PR alone, the context for why this change should happen is completely missing and there is also no issue linked in the PR description (only later in the comment thread are #27244 and #9958 mentioned.
I think there should be a good summary for why you remove this, if you decide to remove this.

I would prefer you kept it, as we at Dynatrace rely on the dynamic_ot tracer to inject our agent. There is no equivalent replacement available that would allow us to execute code that gathers additional process metrics in-process and use our custom exporter/protocol code. Even though we are working on a replacement based on the OpenTelemetry exporter (which is also not yet available in with the OTel-spec-defined default HTTP transport), we would be thankful if you could manage to carry the feature a bit longer.

@phlax
Copy link
Member Author

phlax commented May 10, 2023

as i said - if its a priority - then probably best prioritizing a fix

ive devoted a lot more time to doing so than i intended, and im also offering to help with and review a fix that means we do carry it longer

im not offering to argue about or justify why builds should be hermetic tho

@Oberon00
Copy link
Contributor

I don't want to argue about it, sorry if it came out like that. I just meant that it may be better to reference it in the PR description.

@phlax
Copy link
Member Author

phlax commented May 11, 2023

@kyessenov can we land this for now so we unbreak local devs

im happy to assist readding as explained before the next release if necessary - we can also open a ticket to track

@Oberon00
Copy link
Contributor

Oberon00 commented May 11, 2023

Can you explain how this breaks local dev? I tried to build from main 1bd16ec on Ubuntu (WSL 2) with LLVM 14 and it worked fine for me. What are you using that breaks? Do I need to execute a particular target?

@phlax
Copy link
Member Author

phlax commented May 11, 2023

can you post your build command please

@Oberon00
Copy link
Contributor

Oberon00 commented May 11, 2023

I just used what was described in the readme: bazel build envoy

@phlax
Copy link
Member Author

phlax commented May 11, 2023

are you using the ci/run_envoy_docker.sh container ?

@Oberon00
Copy link
Contributor

No, I couldn't get the VS Code devcontainer to work for me due to networking issues, probably related to WSL 2, so I didn't even try that other one. From what I understand, I might accidentally work around the problem by having CMake installed in my environment?

@phlax
Copy link
Member Author

phlax commented May 11, 2023

I might accidentally work around the problem by having CMake installed in my environment?

correct - or induce the problem we are trying to fix depending on pov

@Oberon00
Copy link
Contributor

Is that the "expected" error?

$ ci/run_envoy_docker.sh
Unable to find image 'envoyproxy/envoy-build-ubuntu:818d28832abf2a7c0cb2bff00435be231729a0bf' locally
818d28832abf2a7c0cb2bff00435be231729a0bf: Pulling from envoyproxy/envoy-build-ubuntu
ca1778b69356: Already exists
b432f54f75b7: Already exists
ed06dd65c63d: Already exists
66a08b852327: Already exists
Digest: sha256:9b04e1cb04fc579142dd820da4583823413b486928941a6c557752a9bcad0608
Status: Downloaded newer image for envoyproxy/envoy-build-ubuntu:818d28832abf2a7c0cb2bff00435be231729a0bf
No cmake here!
bash: -c: line 1: syntax error: unexpected end of file

@phlax
Copy link
Member Author

phlax commented May 11, 2023

Is that the "expected" error?

if you dont give it any commands

probs it could handle this better

@Oberon00
Copy link
Contributor

Thanks, I just found https://github.com/envoyproxy/envoy/tree/main/ci#on-linux, I will look into that

@phlax
Copy link
Member Author

phlax commented May 15, 2023

closing this as the immediate issue has been resolved in #27400

there is still the intention to deprecate and remove this lib - but i will open a separate ticket to track that

@phlax phlax closed this May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants