Open Telemetry by Xaelias · Pull Request #819 · reddit/baseplate.py

Xaelias · 2023-12-22T17:19:53Z

This PR implements otel for http and thrift in bp.py.
Because of unsolvable conflicting dependencies, this also removes python 3.7.

trevorriles · 2024-01-19T17:28:31Z

@chriskuehl you mentioned that there is some changes that have made it to develop but not been released yet. Would it be better for us to target a release branch?

chriskuehl

First pass review -- it would be a good idea to have someone more familiar with the tracing bits also take a look.

trevorriles · 2024-03-19T14:43:55Z

I've got develop merged in and all the tests passing. I'll begin addressing the other PR comments next.

One thing to note is that opentelemetry-python doesn't officially support 3.12 yet, so we may have to wait for that or restrict baseplate to 3.11 for the time being.

trevorriles · 2024-04-02T15:13:28Z

@chriskuehl I'm curious what your thoughts are on the lack of 3.12 support at the moment?

open-telemetry/opentelemetry-python#3617

chriskuehl · 2024-04-02T19:44:59Z

@chriskuehl I'm curious what your thoughts are on the lack of 3.12 support at the moment?

I don't think we can merge this until 3.12 support is available, since we already have services which have migrated to 3.12.

chriskuehl

This looks reasonable but given the size it's difficult to catch any bugs during review. I think we should discuss how we want to test this before the final release.

Let's also get a review from someone more familiar with our tracing setup before merging, since I'm not really qualified to review that beyond basic logic checking.

trevorriles · 2024-04-04T19:05:47Z

This looks reasonable but given the size it's difficult to catch any bugs during review. I think we should discuss how we want to test this before the final release.

Fully on board here, here are two testing steps I want to take before we cut an official release:

Additional testing in snoodev to verify context propagation general functionality
Cut some form of alpha release and work with teams that are still < 3.12. This will make it easier for them to test in snoodev and possibly even some canary testing.

With the 3.12 limitation I'm interested in your thoughts around maintaining an otel branch/release that will be limited to 3.11 and below until we can get the upstream issues resolved?

Let's also get a review from someone more familiar with our tracing setup before merging, since I'm not really qualified to review that beyond basic logic checking.

I'm sure we can get @Xaelias to give this another pass, since I've taken this over. Not sure if you can approve your own PR though. Ultimately we are the two people who have context to the tracing work here. Pete Loaf has done most of the golang work, but I'm not sure if it makes sense to have him review here as well or not?

…agator and log trace IDs

I think we are fine here since we re-raise the exception anyways, if you miss ending a span for exceptions that are not included in I think that is OK.

…n recording option when creating the span

…egers This also shifts to the assumption that we do not truncate the 128 bit trace id but send it as is.

This change is based on the upstream pyramid reify code we orginally lifted this from.

#920) * Move baseplate.py sampling and configuration to be more in line with baseplate.go implementation --------- Co-authored-by: Trevor Riles <trevor@reddit.com>

….lesieur/opentelemetry2

trevorriles · 2024-06-01T20:21:57Z

+    sample_rps = 10
+    try:
+        sample_rps = DefaultFromEnv(Integer, "BASEPLATE_TRACE_SAMPLER_RPS", 10)
+    except ValueError:
+        logger.warning(
+            "Invalid BASEPLATE_TRACE_SAMPLER_RPS, falling back to the default of 10 RPS."
+        )


@chriskuehl I'm trying to use DefaultFromEnv as suggested but I think I'm misunderstanding how this is supposed to work. Does this also require implementing these options in the ini based config to get it to work?

Co-authored-by: redloaf <99754113+redloaf@users.noreply.github.com>

Bumps [lxml](https://github.com/lxml/lxml) from 5.2.1 to 5.2.2. - [Release notes](https://github.com/lxml/lxml/releases) - [Changelog](https://github.com/lxml/lxml/blob/master/CHANGES.txt) - [Commits](lxml/lxml@lxml-5.2.1...lxml-5.2.2) --- updated-dependencies: - dependency-name: lxml dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps [boto3](https://github.com/boto/boto3) from 1.34.95 to 1.34.117. - [Release notes](https://github.com/boto/boto3/releases) - [Changelog](https://github.com/boto/boto3/blob/develop/CHANGELOG.rst) - [Commits](boto/boto3@1.34.95...1.34.117) --- updated-dependencies: - dependency-name: boto3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps [actions/checkout](https://github.com/actions/checkout) from 4.1.4 to 4.1.6. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@0ad4b8f...a5ac7e5) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps [urllib3](https://github.com/urllib3/urllib3) from 1.26.18 to 1.26.19. - [Release notes](https://github.com/urllib3/urllib3/releases) - [Changelog](https://github.com/urllib3/urllib3/blob/1.26.19/CHANGES.rst) - [Commits](urllib3/urllib3@1.26.18...1.26.19) --- updated-dependencies: - dependency-name: urllib3 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps [typing-extensions](https://github.com/python/typing_extensions) from 4.12.0 to 4.12.2. - [Release notes](https://github.com/python/typing_extensions/releases) - [Changelog](https://github.com/python/typing_extensions/blob/main/CHANGELOG.md) - [Commits](python/typing_extensions@4.12.0...4.12.2) --- updated-dependencies: - dependency-name: typing-extensions dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps [moto](https://github.com/getmoto/moto) from 5.0.9 to 5.0.10. - [Release notes](https://github.com/getmoto/moto/releases) - [Changelog](https://github.com/getmoto/moto/blob/master/CHANGELOG.md) - [Commits](getmoto/moto@5.0.9...5.0.10) --- updated-dependencies: - dependency-name: moto dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps [pylint](https://github.com/pylint-dev/pylint) from 3.2.2 to 3.2.5. - [Release notes](https://github.com/pylint-dev/pylint/releases) - [Commits](pylint-dev/pylint@v3.2.2...v3.2.5) --- updated-dependencies: - dependency-name: pylint dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps [flake8](https://github.com/pycqa/flake8) from 7.0.0 to 7.1.0. - [Commits](PyCQA/flake8@7.0.0...7.1.0) --- updated-dependencies: - dependency-name: flake8 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps [types-setuptools](https://github.com/python/typeshed) from 70.0.0.20240524 to 70.1.0.20240627. - [Commits](https://github.com/python/typeshed/commits) --- updated-dependencies: - dependency-name: types-setuptools dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps [boto3](https://github.com/boto/boto3) from 1.34.117 to 1.34.136. - [Release notes](https://github.com/boto/boto3/releases) - [Commits](boto/boto3@1.34.117...1.34.136) --- updated-dependencies: - dependency-name: boto3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps [certifi](https://github.com/certifi/python-certifi) from 2024.2.2 to 2024.7.4. - [Commits](certifi/python-certifi@2024.02.02...2024.07.04) --- updated-dependencies: - dependency-name: certifi dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…ig will come with a future release keeping us aligned with baseplate.go v2

Xaelias force-pushed the aleksei.lesieur/opentelemetry2 branch 2 times, most recently from 538a52b to 6f85007 Compare December 22, 2023 18:02

Xaelias marked this pull request as ready for review December 22, 2023 22:01

Xaelias requested a review from a team as a code owner December 22, 2023 22:01

trevorriles requested review from KTAtkinson and chriskuehl January 19, 2024 17:27

trevorriles self-assigned this Jan 19, 2024

chriskuehl reviewed Jan 19, 2024

View reviewed changes

chriskuehl mentioned this pull request Jan 19, 2024

Drop python3.7 support #824

Merged

3 tasks

KTAtkinson suggested changes Jan 22, 2024

View reviewed changes

Comment thread baseplate/clients/requests.py

trevorriles requested review from KTAtkinson and chriskuehl April 2, 2024 15:13

KTAtkinson approved these changes Apr 4, 2024

View reviewed changes

chriskuehl approved these changes Apr 4, 2024

View reviewed changes

trevorriles reviewed Apr 4, 2024

View reviewed changes

Comment thread baseplate/server/__init__.py Outdated

Xaelias and others added 10 commits May 6, 2024 10:05

feat: adhoc: Update pre-commit hooks and Dockerfile for local dev

3906dd1

feat: adhoc: Install opentelemetry pacakges, create Reddit trace prop…

c89b58e

…agator and log trace IDs

feat: adhoc: Implement opentelemetry for thrift client and server

8875362

feat: adhoc: Instrument pyramid/request with Opentelemetry

ca26600

trying to move the auto instrumentation around as requested

3603ce0

Addressed some PR comments

82405bc

Change these back from BaseException to Exception

3ccd636

I think we are fine here since we re-raise the exception anyways, if you miss ending a span for exceptions that are not included in I think that is OK.

Switch thrift back to and record the exception to otel.

46459d9

Don't explicitly record the exception as we enabled the auto exceptio…

1213039

…n recording option when creating the span

Update propagator to correctly follow baseplate spec using base10 int…

2382d2e

…egers This also shifts to the assumption that we do not truncate the 128 bit trace id but send it as is.

Trevor Riles and others added 8 commits May 6, 2024 11:04

Update cached_property wrapper

3905041

This change is based on the upstream pyramid reify code we orginally lifted this from.

Add back in baseplate thrift errorcode fix

a5c750a

Fix indentation error made during rebase

f194423

Set doc pyproject black language level to match top-level project

f35abeb

Fix mypy type that was missed during rebase

4ca3656

Remove reset_metrics method in tests that did not call it

62c8347

Move baseplate.py sampling and configuration to be more in line with … (

5216cfe

#920) * Move baseplate.py sampling and configuration to be more in line with baseplate.go implementation --------- Co-authored-by: Trevor Riles <trevor@reddit.com>

Merge branch 'develop' of github.com:reddit/baseplate.py into aleksei…

7969d5a

….lesieur/opentelemetry2

trevorriles approved these changes Jun 1, 2024

View reviewed changes

make fmt

9834885

trevorriles reviewed Jun 1, 2024

View reviewed changes

trevorriles requested a review from redloaf June 3, 2024 15:43

redloaf approved these changes Jun 5, 2024

View reviewed changes

Comment thread baseplate/frameworks/thrift/__init__.py Outdated

trevorriles and others added 15 commits June 6, 2024 10:06

Update baseplate/frameworks/thrift/__init__.py

e478766

Co-authored-by: redloaf <99754113+redloaf@users.noreply.github.com>

v2.7.0

4b9af5d

Remove broken environment variable based configuration. Sampling conf…

b925e67

…ig will come with a future release keeping us aligned with baseplate.go v2

Remove unused imports

cf426c7

trevorriles merged commit 3423a65 into develop Jul 23, 2024

trevorriles deleted the aleksei.lesieur/opentelemetry2 branch July 23, 2024 18:37

Conversation

Xaelias commented Dec 22, 2023

Uh oh!

trevorriles commented Jan 19, 2024

Uh oh!

chriskuehl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

trevorriles commented Mar 19, 2024

Uh oh!

trevorriles commented Apr 2, 2024

Uh oh!

chriskuehl commented Apr 2, 2024

Uh oh!

chriskuehl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

trevorriles commented Apr 4, 2024

Uh oh!

Uh oh!

trevorriles Jun 1, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants