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

Implement setOperation for the opentelemetry provider #34062

Merged
merged 2 commits into from May 13, 2024

Conversation

therealak12
Copy link
Contributor

@therealak12 therealak12 commented May 9, 2024

Commit Message: implement setOperation for the opentelemetry provider

Additional Description:
The detailed description is provided in issue#34063. In short, the MR implements a method for the OpenTelemetry tracing provider that was left empty and thus made the setOperation method a no-op.

Risk Level: Low

Testing: Unit testing and the sandbox.

Fixes issue#34063

Signed-off-by: Ahmad Karimi <ak12hastam@gmail.com>
@therealak12 therealak12 requested a review from htuch as a code owner May 9, 2024 18:07
Copy link

Hi @therealak12, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #34062 was opened by therealak12.

see: more, trace.

@therealak12
Copy link
Contributor Author

therealak12 commented May 9, 2024

The ci/do_ci.sh verify_examples check failed likely due to the following error. I'd appreciate any help on how to fix it.

#6 [wasm_compile_update stage-0 2/2] RUN --mount=type=cache,target=/var/cache/apt,sharing=locked     --mount=type=cache,target=/var/lib/apt/lists,sharing=locked     apt-get -qq install --no-install-recommends -y gosu     && groupadd -f envoygroup     && useradd -g envoygroup -m -d /home/envoybuild envoybuild
#6 0.224 E: Unable to locate package gosu
#6 ERROR: process "/bin/bash -ec apt-get -qq install --no-install-recommends -y gosu     && groupadd -f envoygroup     && useradd -g envoygroup -m -d /home/envoybuild envoybuild" did not complete successfully: exit code: 100
------
 > [wasm_compile_update stage-0 2/2] RUN --mount=type=cache,target=/var/cache/apt,sharing=locked     --mount=type=cache,target=/var/lib/apt/lists,sharing=locked     apt-get -qq install --no-install-recommends -y gosu     && groupadd -f envoygroup     && useradd -g envoygroup -m -d /home/envoybuild envoybuild:
0.224 E: Unable to locate package gosu
------
failed to solve: process "/bin/bash -ec apt-get -qq install --no-install-recommends -y gosu     && groupadd -f envoygroup     && useradd -g envoygroup -m -d /home/envoybuild envoybuild" did not complete successfully: exit code: 100

@phlax
Copy link
Member

phlax commented May 10, 2024

its actually a slightly different error (single-page-app) - the one posted above is known flakey so is caught as a warning - not sure exactly the cause

/retest

@therealak12
Copy link
Contributor Author

therealak12 commented May 10, 2024

Thanks for rerunning the Publish and verify job. Now The CodeQL/push... job is failing with java.io.IOException: No space left on device.

/retest

@therealak12 therealak12 reopened this May 10, 2024
@therealak12
Copy link
Contributor Author

therealak12 commented May 10, 2024

Haha. 😂
Inadvertently closed and reopened the pull request.
I didn't know it would result in rerunning the jobs.
By the way, the checks are all passed. 🥳

@phlax
Copy link
Member

phlax commented May 10, 2024

@therealak12 a sandbox test is helpful also, but probably this shoud have some code test if its critical to functionality

@therealak12
Copy link
Contributor Author

therealak12 commented May 10, 2024

@phlax
It depends on how we define critical.
Nonetheless, being compatible with Envoy docs is important enough to have tests for.
So, thanks for reminding me. I'll add proper tests.

@therealak12
Copy link
Contributor Author

I'm going to mark the PR as draft till I add the tests.

@therealak12 therealak12 marked this pull request as draft May 10, 2024 17:05
Signed-off-by: Ahmad Karimi <ak12hastam@gmail.com>
@therealak12
Copy link
Contributor Author

/retest

@therealak12
Copy link
Contributor Author

therealak12 commented May 11, 2024

@phlax
I added a new test for the intended scenario.
Marking the PR as ready for review.

@therealak12 therealak12 marked this pull request as ready for review May 11, 2024 16:39
@therealak12
Copy link
Contributor Author

Dear @htuch;
Kindly please let me know if there's anything else I can do to move forward with the review process.

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@htuch htuch merged commit 1a1adb8 into envoyproxy:main May 13, 2024
52 checks passed
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.

Envoy doesn't set span name, aka operation name, when using the OpenTelemetry tracing provider
3 participants