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

feat(sdk): chained entity path on nested tasks #1782

Merged

Conversation

OzBenSimhonTraceloop
Copy link
Contributor

@OzBenSimhonTraceloop OzBenSimhonTraceloop commented Aug 5, 2024

  • I have added tests that cover my changes.
  • If adding a new instrumentation or changing an existing one, I've added screenshots from some observability platform showing the change.
  • PR name follows conventional commits format: feat(instrumentation): ... or fix(instrumentation): ....
  • (If applicable) I have updated the documentation accordingly.

Example

Assuming we have a workflow with three nesting levels of tasks

some_workflow 
=> outer_task 
    => inner_task
         =>  inner_inner_task

The expected entity paths will be

  • some_workflow path is None
  • outer_task path is 'outer_task'
  • inner_task path is 'outer_task.inner_task'
  • inner_inner_task path is 'outer_task.inner_task.inner_inner_task'

@CLAassistant
Copy link

CLAassistant commented Aug 5, 2024

CLA assistant check
All committers have signed the CLA.

@OzBenSimhonTraceloop OzBenSimhonTraceloop changed the title [FEAT] Chained entity path instead of chained entity name chore: chained entity path on nested tasks Aug 6, 2024
@OzBenSimhonTraceloop OzBenSimhonTraceloop marked this pull request as ready for review August 6, 2024 05:47
Copy link
Member

@nirga nirga left a comment

Choose a reason for hiding this comment

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

That's a breaking change, let's wait with it for later this week as I want to release a small fix today.
Left a nit comment

@@ -10,65 +10,32 @@
Traceloop.init(app_name="joke_generation_service")


@task(name="joke_creation", version=1)
Copy link
Member

Choose a reason for hiding this comment

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

Why did you delete this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I copied this file from Gal branch, I just saw he excluded it from his merged PR, so I'll remove this change as well 🙈

@nirga nirga changed the title chore: chained entity path on nested tasks fix(sdk): chained entity path on nested tasks Aug 6, 2024
@nirga nirga changed the title fix(sdk): chained entity path on nested tasks feat(sdk): chained entity path on nested tasks Aug 6, 2024
@OzBenSimhonTraceloop OzBenSimhonTraceloop merged commit 826a101 into main Aug 7, 2024
8 checks passed
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.

4 participants