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.

Sign up for GitHub

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

Jump to bottom

Otel oss integration #1462

Merged
merged 10 commits into from Oct 25, 2023
Merged

Otel oss integration #1462

merged 10 commits into from Oct 25, 2023

Conversation

wangchy27
Copy link
Collaborator

No description provided.

@nflx-mf-bot
Copy link
Collaborator

Testing[437] @ 8c81441

@nflx-mf-bot
Copy link
Collaborator

Testing[437] @ 8c81441

@nflx-mf-bot
Copy link
Collaborator

Testing[437] @ 8c81441 had 15 FAILUREs.

saikonen
saikonen reviewed Jun 28, 2023
View reviewed changes
metaflow/cmd/configure_cmd.py Outdated Show resolved Hide resolved
metaflow/metadata/heartbeat.py Show resolved Hide resolved
metaflow/mflog/save_logs.py Show resolved Hide resolved
metaflow/plugins/datatools/s3/s3.py Show resolved Hide resolved
metaflow/plugins/metadata/service.py Show resolved Hide resolved
saikonen
saikonen previously approved these changes Jul 14, 2023
View reviewed changes
saikonen
saikonen reviewed Aug 17, 2023
View reviewed changes
metaflow/plugins/kubernetes/kubernetes_job.py
]
+ [
client.V1EnvVar(name=k, value=str(v))
for k, v in inject_tracing_vars({}).items()
Copy link
Collaborator

@saikonen saikonen Aug 17, 2023 •
edited

Choose a reason for hiding this comment

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

This seems to lead to unnecessary import errors in the task logs

No module named 'opentelemetry'

when running with Kubernetes

Copy link
Collaborator

Choose a reason for hiding this comment

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

only happens when trying to enable telemetry, but not including required libraries with either conda / a custom docker image. I think this is a good notification as is so no need to change things.

@nflx-mf-bot
Copy link
Collaborator

Testing[437] @ 6f8af34

@nflx-mf-bot
Copy link
Collaborator

Testing[437] @ 6f8af34 had 1 FAILURE.

@nflx-mf-bot
Copy link
Collaborator

Testing[437] @ a0ed23f

@nflx-mf-bot
Copy link
Collaborator

Testing[437] @ a0ed23f had 3 FAILUREs.

@wangchy27
Copy link
Collaborator Author

wangchy27 commented Sep 14, 2023 •
edited

The changes are straightforward and make a lot of sense. Thanks @saikonen for handling this!
I can't approve the PR since I'm the owner. @romain-intel Could you take a quick look and approve it?

@wangchy27 wangchy27 assigned wangchy27 and unassigned wangchy27 Sep 14, 2023
@nflx-mf-bot
Copy link
Collaborator

Testing[437] @ a0ed23f

@nflx-mf-bot
Copy link
Collaborator

Testing[437] @ a0ed23f had 2 FAILUREs.

@romain-intel
Copy link
Contributor

Internal tests are fine (the 2 failures are flaky). The R test I don't know.

romain-intel
romain-intel previously approved these changes Sep 14, 2023
View reviewed changes
@saikonen saikonen merged commit 6835bff into master Oct 25, 2023
20 checks passed
@saikonen saikonen deleted the otel_oss_integration branch October 25, 2023 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oavdeev oavdeev oavdeev left review comments

@savingoyal savingoyal savingoyal left review comments

@saikonen saikonen saikonen left review comments

@romain-intel romain-intel romain-intel left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@wangchy27 @nflx-mf-bot @romain-intel @oavdeev @savingoyal @saikonen