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

Improve logging with open telemetry traces #9083

Merged
merged 13 commits into from
Nov 1, 2024

Conversation

HakonSohoel
Copy link
Contributor

@HakonSohoel HakonSohoel commented Oct 29, 2024

Adds a top level trace to ert.
Add a span processor through the add_span_processor pluggin hook to export the trace to e.g. azure

Issue
Resolves #8588

Approach
Short description of the approach

(Screenshot of new behavior in GUI if applicable)

  • PR title captures the intent of the changes, and is fitting for release notes.
  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Make sure unit tests pass locally after every commit (git rebase -i main --exec 'pytest tests/ert/unit_tests -n logical -m "not integration_test"')

When applicable

  • When there are user facing changes: Updated documentation
  • New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
  • Large PR: Prepare changes in small commits for more convenient review
  • Bug fix: Add regression test for the bug
  • Bug fix: Create Backport PR to latest release

Adds a top level trace to ert.
Add a span processor through the add_span_processor pluggin hook
to export the trace to e.g. azure
@HakonSohoel HakonSohoel marked this pull request as draft October 29, 2024 13:00
@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2024

Codecov Report

Attention: Patch coverage is 92.42424% with 5 lines in your changes missing coverage. Please review.

Project coverage is 90.72%. Comparing base (5f07ccb) to head (4c0127f).
Report is 28 commits behind head on main.

Files with missing lines Patch % Lines
src/ert/__main__.py 71.42% 4 Missing ⚠️
src/ert/plugins/plugin_manager.py 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9083      +/-   ##
==========================================
+ Coverage   90.69%   90.72%   +0.02%     
==========================================
  Files         349      351       +2     
  Lines       21768    21834      +66     
==========================================
+ Hits        19742    19808      +66     
  Misses       2026     2026              
Flag Coverage Δ
cli-tests 39.10% <71.21%> (+0.12%) ⬆️
gui-tests 71.70% <71.21%> (+<0.01%) ⬆️
performance-tests 49.29% <68.18%> (+0.03%) ⬆️
unit-tests 79.60% <92.42%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@HakonSohoel HakonSohoel added the release-notes:logging PR which only changes logging. label Oct 31, 2024
@HakonSohoel HakonSohoel changed the title Use spans for logging Improve logging with open telemetry traces Oct 31, 2024
@HakonSohoel HakonSohoel marked this pull request as ready for review October 31, 2024 12:40
@HakonSohoel HakonSohoel enabled auto-merge (squash) October 31, 2024 12:48
Comment on lines 126 to 127
with tracer.start_as_current_span("test_span"):
print("test_span")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe best to use different strings for these? You are testing the logged content of the span, not the name of the span.
Also, would it be possible to create two spans, and log to both, seeing that they would separate logging as intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the strings, and added an extra span. The test seeks to confirm that the add_span_processor plugin hook works, by confirming that span information is written to the span_output object of the dummy_plugins module.

self.started.clear()
else:
await self._send(JobState.FAILED)
with tracer.start_as_current_span(f"{__name__}.run.realization_{self.iens}"):
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we might be a little lucky here since the scope will cover most, if not all, of the functions in this class. 👍


resource = Resource(attributes={SERVICE_NAME: "ert"})
tracer_provider = TracerProvider(
resource=resource, span_limits=SpanLimits(max_events=128 * 16)
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect this number is much higher than what we really need, but let's leave it like this for the time being.
Events within a span are meant for noticeable changes like exceptions, outcomes and such.

Copy link
Contributor

@andreas-el andreas-el left a comment

Choose a reason for hiding this comment

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

Great work! 🏖️

@HakonSohoel HakonSohoel merged commit c34060c into equinor:main Nov 1, 2024
56 checks passed
@HakonSohoel HakonSohoel deleted the Use-spans-for-logging branch November 4, 2024 06:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:logging PR which only changes logging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use spans for logging
3 participants