Implemented get_name in StatsLogger, updated Otel and StatsD #43340
Conversation
- Added get_name method in StatsLogger (base_stat_logger.py) to handle metric name preparation and tag validation. - Updated Otel and StatsD loggers to inherit from StatsLogger and utilize the new get_name method. - Refactored tag preparation and validation logic into get_name for a cleaner and more consistent implementation.
|
@ferruzzi, @howardyoo, @o-nikolas Can you please take a look at it? If anything needs to change please let me know. |
@ArshiaZr, I am not a reviewer, and thus cannot review this PR, but I believe it would be nice if you can add unit tests for your changes as part of this PR, so that the changes can be properly checked to work properly. cc to @ferruzzi |
Actually - anyone can review PRs - and we encourage it. You can even approve it @howardyoo and it might guide other maintainers with their approvals. |
|
No, as far as I know, I don’t think I have the ability to approve the PR, but if I get a chance I’ll sure do the review. Having the unit test would be very helpful for the testing too.Sent from my iPhoneOn Oct 25, 2024, at 12:11 PM, Jarek Potiuk ***@***.***> wrote:
@ArshiaZr, I am not a reviewer, and thus cannot review this PR, but I believe it would be nice if you can add unit tests for your changes as part of this PR, so that the changes can be properly checked to work properly. cc to @ferruzzi
Actually - anyone can review PRs - and we encourage it. You can even approve it @howardyoo and it might guide other maintainers with their approvals.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
You can approve it (many contributors do that). But you need at least one approval from a commiter to merge it - but approvals can be done by absolutely anyone who has a GitHub account. |
|
Ok, I see. Thanks!Sent from my iPhoneOn Oct 25, 2024, at 12:26 PM, Jarek Potiuk ***@***.***> wrote:
No, as far as I know, I don’t think I have the ability to approve the PR
You can approve it (many contributors do that). But you need at least one approval from a commiter to merge it - but approvals can be done by absolutely anyone who has a GitHub account.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
I reviewed the code, and to me, it is well implemented and ready to be
merged.
…On Fri, Oct 25, 2024 at 12:30 PM Howard Yoo ***@***.***> wrote:
Ok, I see. Thanks!
Sent from my iPhone
On Oct 25, 2024, at 12:26 PM, Jarek Potiuk ***@***.***>
wrote:
No, as far as I know, I don’t think I have the ability to approve the PR
You can approve it (many contributors do that). But you need at least one
approval from a commiter to merge it - but approvals can be done by
absolutely anyone who has a GitHub account.
—
Reply to this email directly, view it on GitHub
<#43340 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHZNLLU25JM2URXAA4NZZRLZ5J5M7AVCNFSM6AAAAABQQJT7UOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMZYGQYDEMRYGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Do we expect any incompatibilities in produced metrics @ArshiaZr ? I would love to understand what was the intention of "cleaner and more consistent implementation." before I get into details. |
| @@ -31,55 +33,53 @@ class StatsLogger(Protocol): | |||
|
|
|||
| instance: StatsLogger | NoStatsLogger | None = None | |||
|
|
|||
| @classmethod | |||
There was a problem hiding this comment.
Why are we removing @classmethod from all of these? Seems unusual to remove the decorator but leave the cls parameter.
There was a problem hiding this comment.
I'd be happy for an explanation about the necessity of removing @classmethod - won't it break the functionality that we have in dag_processing/manager.py? For example:
…tsdLogger for enhanced validation - Modified StatsLogger base class to enforce stricter validation on metric names and prefixes, ensuring all names are strings and conform to required formats. - Enhanced SafeOtelLogger with additional validation to support OpenTelemetry standards and added regression tests for metrics with and without tags. - Updated SafeStatsdLogger to align with new validation standards and to handle edge cases more robustly. - Added regression tests to catch issues with missing tags, empty names, and overly long names, ensuring consistency across changes.
|
I just made the code more robust based on @ferruzzi reviews I'd appreciate it if you could look at it. cc @howardyoo, @dannyl1u |
|
@ferruzzi Thanks for the reviews. Here's the fixed version. I had to add the instance back in StatsLogger as I noticed we're using it somewhere else. |
|
I raised another commit. I tried to address all the issues and reviewed it myself multiple times. |
The Pydantic 2.10.0/2.10.1 workaround implemented in apache#44317 is not needed any more as it has been fixed in Pydantic 2.10.2 in the pydantic/pydantic#10962 This PR removes the workaround and bumps min Pydantic version to 2.10.2.
* init * wip * remove logical_date * fix trigger dag_run * tests WIP * working tests * remove logical_date from post body * remove logical_date from tests * fix * include return type * fix conf * feedback * fix tests * Update tests/api_fastapi/core_api/routes/public/test_dag_run.py * feedback
Signed-off-by: Maciej Obuchowski <[email protected]>
I intentionally omitted some arguments e.g. start_date. We can figure out what to do with those later. I think these arguments are mostly considered reasonable and not something we wish we didn't add?
… Airflow (apache#44397) In dagcode, fileloc_hash column was a primary key on the version before dag versioning. If dagcode is not deleted before downgrading, the fileloc_hash column, created when downgrading, would be a null column that can't be used for a primary key. A similar thing applies to serialized dag, which would have duplicates, and the primary key can't be determined. The solution is to delete the dag_version table data, which will delete the serdag and dagcode table data
…4389) Deleting a dag would cascade delete dag_version table along with serialized_dag model and dagcode. However, we should be able to delete dag_version directly. I didn't add dag_code or serdag model because it won't make sense to have an existing dag_version without a corresponding dagcode and serdag
…e#44378) * docs(newsfragment): add template for significant newsfragments Closes: apache#44374 * docs(contributing-docs): add sigificant file template description * ci(github-actions): add script to check the news fragment content
* Refactor get_connections * Allow Column type for `to_replace` parameter * Refactor get_dags * Refactor get_import_errors * Refactor SortParam, get_dag_runs * Fix default ordering when directly using SortParam - related: apache#44393 * Fix get_list_dag_runs_batch
We mostly use ruff everywhere, but there is one use-case where black, or specifically blacken-docs is still needed.
Signed-off-by: Kacper Muda <[email protected]>
* move openapi tests to breeze container * remove space * move openapi tests inside special tests workflow * merge openapi tests into special tests workflow * use postgres backend for openapi tests * ignore tmp folder tests discovery in openapi tests * adding retries to python client tests connection * adding retries to python client tests connection * export auth backend configs * use connectivity check to verify webserver started or not * rename is-special-tests-require to special-tests-required * rename openapi-tests to python-api-client-tests * rename static-checks-mypy-docs.yml to ci-image-checks.yml * rename static-checks-mypy-docs.yml to ci-image-checks.yml * remove special-tests-required param
…local (apache#44417) Signed-off-by: Kacper Muda <[email protected]>
* Refactor test_dag_run * Refactor test_assets * Rename datetime_zulu_format to from_datetime_to_zulu * Fix TestListDagRunsBatch
|
I've seen so many iterations on this it's likely a good idea to have a fresh set of eyes have a quick look, but I think it's right and all the tests are green. Congrats on that milestone. |
| @@ -31,55 +33,53 @@ class StatsLogger(Protocol): | |||
|
|
|||
| instance: StatsLogger | NoStatsLogger | None = None | |||
|
|
|||
| @classmethod | |||
There was a problem hiding this comment.
I'd be happy for an explanation about the necessity of removing @classmethod - won't it break the functionality that we have in dag_processing/manager.py? For example:
|
Hi, I'm just curious .. will this pr fix the |
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.