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

[native] Add two new parameters for trace control #24209

Merged

Conversation

zation99
Copy link
Contributor

@zation99 zation99 commented Dec 5, 2024

Description

This is to improve the trace control experience. We added two new properties:

  • native_query_trace_fragment_id
  • native_query_trace_shard_id
    Once specified, we automatically create a regex for Velox to match the taskID pattern, so developers don't need to compose the complicated regex themselves, and it is controlled at Prestissimo level.
    If native_query_trace_task_reg_exp is also provided, it will be overridden.

Motivation and Context

Impact

Test Plan

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==
* Added native_query_trace_fragment_id and native_query_trace_shard_id for easier trace control:pr:`24209`

@zation99 zation99 requested a review from xiaoxmeng December 5, 2024 18:55
@zation99 zation99 requested review from a team as code owners December 5, 2024 18:55
@zation99 zation99 requested a review from presto-oss December 5, 2024 18:55
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@zation99 looks good % comments. We will deprecate the existing session property for task regular expr in followup? Thanks!

@xiaoxmeng xiaoxmeng changed the title Add two new parameters for trace control [native] Add two new parameters for trace control Dec 6, 2024
@steveburnett
Copy link
Contributor

Please add documentation for these properties to the appropriate pages:
Presto Session Properties
Presto Configuration Properties
Presto C++ Session Properties
Presto C++ Configuration Properties

xiaoxmeng
xiaoxmeng previously approved these changes Dec 7, 2024
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@zation99 LGTM. Thanks!

@zation99
Copy link
Contributor Author

zation99 commented Dec 7, 2024

Please add documentation for these properties to the appropriate pages: Presto Session Properties Presto Configuration Properties Presto C++ Session Properties Presto C++ Configuration Properties

done

xiaoxmeng
xiaoxmeng previously approved these changes Dec 8, 2024
Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

@zation99 : Please can you merge all your commits into a single one with the same title as this PR.

@zation99
Copy link
Contributor Author

zation99 commented Dec 9, 2024

@zation99 : Please can you merge all your commits into a single one with the same title as this PR.

Yes I'll do "Squash and merge" when merging the PR.
Update: nvm I just squashed them manually and resubmitted.

@zation99 zation99 force-pushed the jiaqizhang_t205994517_adjust_trace_control branch from 16d14fd to abcb06c Compare December 9, 2024 18:03
@zation99 zation99 added the from:Meta PR from Meta label Dec 9, 2024
@prestodb-ci
Copy link

Saved that user @zation99 is from Meta

@zation99
Copy link
Contributor Author

@zation99 : Please can you merge all your commits into a single one with the same title as this PR.

@zation99 zation99 closed this Dec 10, 2024
@zation99 zation99 reopened this Dec 10, 2024
@zation99
Copy link
Contributor Author

@zation99 : Please can you merge all your commits into a single one with the same title as this PR.

Yes I'll do "Squash and merge" when merging the PR. Update: nvm I just squashed them manually and resubmitted.

@aditi-pandit do you have any other comment?

Copy link
Contributor

@aditi-pandit aditi-pandit left a comment

Choose a reason for hiding this comment

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

Thanks @zation99

@zation99 zation99 merged commit 8ed1609 into prestodb:master Dec 10, 2024
111 checks passed
denodo-research-labs pushed a commit to denodo-research-labs/presto that referenced this pull request Dec 12, 2024
Add two new parameters for trace control
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
from:Meta PR from Meta
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants