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

Instrument the transformer using the opentelemetry-sdk #727

Merged
merged 10 commits into from
Sep 13, 2023

Conversation

miyunari
Copy link
Member

@miyunari miyunari commented Aug 28, 2023

This PR instruments:

  • the v2 provider transformer
  • the v2 list all endpoint transformer
  • all Image filters

While I have still an open question how to call the counter add method when data gets filtered using lambda.

As a next step, we have to add a otel exporter to store and visualize the data.

closes #728

@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.38% 🎉

Comparison is base (326229b) 92.85% compared to head (019d968) 93.23%.
Report is 1 commits behind head on main.

❗ Current head 019d968 differs from pull request most recent head d5e1196. Consider uploading reports for the commit d5e1196 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #727      +/-   ##
==========================================
+ Coverage   92.85%   93.23%   +0.38%     
==========================================
  Files           9        9              
  Lines         532      562      +30     
  Branches      110      112       +2     
==========================================
+ Hits          494      524      +30     
  Misses         21       21              
  Partials       17       17              
Files Changed Coverage Δ
src/cloudimagedirectory/transformer.py 77.77% <ø> (ø)
src/cloudimagedirectory/filter/filter.py 100.00% <100.00%> (ø)
src/cloudimagedirectory/transform/transform.py 91.97% <100.00%> (+0.20%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@miyunari miyunari marked this pull request as ready for review August 28, 2023 08:12
@miyunari miyunari requested a review from poncovka August 30, 2023 14:11
Copy link

@frzifus frzifus left a comment

Choose a reason for hiding this comment

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

I'm no expert at naming things, but do the suggestions make sense? Otherwise LGTM!

Here is a link to the convention: otel-metrics-semantic-conventions

src/cloudimagedirectory/filter/filter.py Outdated Show resolved Hide resolved
src/cloudimagedirectory/filter/filter.py Outdated Show resolved Hide resolved
src/cloudimagedirectory/filter/filter.py Outdated Show resolved Hide resolved
src/cloudimagedirectory/transform/transform.py Outdated Show resolved Hide resolved
src/cloudimagedirectory/transform/transform.py Outdated Show resolved Hide resolved
@miyunari miyunari force-pushed the instrument_otel branch 3 times, most recently from 6e32f6c to 6e1f652 Compare September 12, 2023 13:01
Signed-off-by: Janine Olear <[email protected]>
Copy link
Member

@F-X64 F-X64 left a comment

Choose a reason for hiding this comment

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

This is a great foundation, I can't wait to see the rest of the implementation! I only got some really minor questions that have no impact on functionality. Feel free to merge once you're ready.

@@ -518,7 +540,7 @@ class TransformerV2ListVersionByProvider(TransformerV2):
"""Generate a list for all available versions for a specific provider."""

def run(self, data: list[DataEntry]) -> list:
# Start each version at a count of 0 so we can increment the counter as
Copy link
Member

Choose a reason for hiding this comment

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

I guess this was removed by mistake?

Copy link
Member Author

Choose a reason for hiding this comment

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

lol - yes you are right! :D

@@ -41,6 +41,8 @@
help="List of predefined files to process",
)
def run(origin_path: str, destination_path: str, arg_files: str, filter_until: str) -> None:
# metrics.get_meter_provider().start_pipeline(ConsoleMetricExporter(), interval=5)
Copy link
Member

Choose a reason for hiding this comment

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

Is this a placeholder for future implementations?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, the next step is to export the data.

@@ -518,7 +540,7 @@ class TransformerV2ListVersionByProvider(TransformerV2):
"""Generate a list for all available versions for a specific provider."""

def run(self, data: list[DataEntry]) -> list:
# Start each version at a count of 0 so we can increment the counter as
Copy link
Member Author

Choose a reason for hiding this comment

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

lol - yes you are right! :D

src/cloudimagedirectory/transform/transform.py Outdated Show resolved Hide resolved
@miyunari miyunari merged commit cd4aafd into redhatcloudx:main Sep 13, 2023
4 checks passed
@miyunari miyunari deleted the instrument_otel branch September 13, 2023 14:13
Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

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

Late, but it looks good to me :-)

@miyunari miyunari self-assigned this Sep 19, 2023
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.

Instrumentation of OpenTelemetry
4 participants