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

[SYNPY-1304] Introduction of OpenTelemetry #1007

Merged
merged 13 commits into from
Nov 3, 2023

Conversation

BryanFauble
Copy link
Contributor

Background SPIKE:

  1. https://sagebionetworks.jira.com/browse/SYNPY-1290
  2. Confluence Doc: https://sagebionetworks.jira.com/wiki/spaces/IBC/pages/3041099777/SYNPY-1289+Open+Telemetry+Metrics+OTEL+in+synapse+python+client

Solution:
This PR introduces 3 new dependencies to the Synapse Python project around OpenTelemetry. I have also manually instrumented the code in areas where it would be programmatically entered, and any areas where I/O is expected to occur for communication with our Rest APIs.

Please carefully review the changes to the README around this and let me know areas you'd like it improved.

JaegerUI to view traces:
image

@BryanFauble BryanFauble requested a review from BWMac November 2, 2023 23:00
@BryanFauble BryanFauble requested a review from a team as a code owner November 2, 2023 23:00
@@ -63,7 +63,7 @@ jobs:
path: |
${{ steps.get-dependencies.outputs.site_packages_loc }}
${{ steps.get-dependencies.outputs.site_bin_dir }}
key: ${{ runner.os }}-${{ matrix.python }}-build-${{ env.cache-name }}-${{ hashFiles('setup.py') }}-v7
key: ${{ runner.os }}-${{ matrix.python }}-build-${{ env.cache-name }}-${{ hashFiles('setup.py') }}-v7-otel
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 be reverted before merge - This is to allow the CICD pipeline to install the new dependencies and not used the cached version

@pep8speaks
Copy link

pep8speaks commented Nov 2, 2023

Hello @BryanFauble! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 245:89: E501 line too long (105 > 88 characters)

Line 621:89: E501 line too long (106 > 88 characters)
Line 651:89: E501 line too long (104 > 88 characters)
Line 716:89: E501 line too long (114 > 88 characters)
Line 799:89: E501 line too long (118 > 88 characters)

Line 758:89: E501 line too long (96 > 88 characters)

Line 99:89: E501 line too long (102 > 88 characters)
Line 120:89: E501 line too long (101 > 88 characters)
Line 134:89: E501 line too long (109 > 88 characters)

Line 418:89: E501 line too long (93 > 88 characters)

Comment last updated at 2023-11-03 16:13:56 UTC

README.md Outdated Show resolved Hide resolved
Copy link
Member

@thomasyu888 thomasyu888 left a comment

Choose a reason for hiding this comment

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

🔥 This looks great - id say since this is the intro to open telemetry, its ok for this PR to be so big. We can take the sprint review to show everyone how this works.

@BryanFauble BryanFauble merged commit a81545f into develop Nov 3, 2023
@BryanFauble BryanFauble deleted the SYNPY-1304-otel-in-synpy branch November 3, 2023 20:16
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.

3 participants