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

Remove "experimental" banner for OTel Metrics #40286

Merged
merged 4 commits into from
Jun 26, 2024

Conversation

ferruzzi
Copy link
Contributor

@ferruzzi ferruzzi commented Jun 17, 2024

OpenTelemetry metrics have been live for a while now and there have been no major bugs except the ongoing discussions around timers reporting in seconds or milliseconds (example). I propose we drop the "Experimental" banner for OTel Metrics and leave the new Experimental for OTel Traces.

@howardyoo
@potiuk
@romanzdk
@o-nikolas

closes: #40218

@boring-cyborg boring-cyborg bot added area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Jun 17, 2024
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

If we make it stable we need also newsfragmant to change log

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Yep. Agree with Elad on the newsfragment adding. Also would be worth checking if there are no other places where otel is marked as experimental (if not done already).

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

Usually, we start lazy voting on the dev mailing list to merge similar PRs.

@ferruzzi
Copy link
Contributor Author

I'll get the voting thread going and add the news fragment, thanks folks.

@ferruzzi
Copy link
Contributor Author

@hussein-awala @potiuk - No comments on the lazy consensus email after 7 days. I think we're good?

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Absolutely. I also think we should remove it from traces - it will have the same problem :)

@ferruzzi
Copy link
Contributor Author

I thought we discussed that and decided to do Traces separately... but now I don't see it in this thread so maybe I made that up. Unfortunately, my lazy consensus email explicitly stated it was for OTel Metrics, so I'll need a new consensus post which would mean leaving this open for another week. Up to you, I'm not aware of any particular time-sensitive reason to rush this so either way works for me.

@potiuk
Copy link
Member

potiuk commented Jun 26, 2024

Oh yes. It could / should be separate PR.

@potiuk potiuk merged commit d5c1fc2 into apache:main Jun 26, 2024
49 checks passed
@ferruzzi
Copy link
Contributor Author

I've got a bunch of meetings today but I'll squeeze in time to get the LC email out for the Traces and I'll own that as well, once that clears the waiting period.

@utkarsharma2 utkarsharma2 added type:doc-only Changelog: Doc Only type:misc/internal Changelog: Misc changes that should appear in change log and removed type:doc-only Changelog: Doc Only labels Jul 1, 2024
@DjVinnii
Copy link
Contributor

DjVinnii commented Jul 2, 2024

@utkarsharma2 I thought this PR was going to be cherry-picked for Airflow 2.9.3 (See last comment of @potiuk in the discussion #40218). Is there a specific reason this is now moved to 2.10? We would like to migrate to OTel metrics, however this banner is still blocking us as we don't want to show that in our production environments.

@potiuk
Copy link
Member

potiuk commented Jul 2, 2024

Yeah. I think it would be great to include that one. Really the banner was a mistake - it is enough to describe feature as experimental, showing a banner is a bad idea, because if someone turns it on, they do it deliberately and do not want to see the banner. So we can easily treat that one as "bugfix".

@utkarsharma2
Copy link
Contributor

utkarsharma2 commented Jul 2, 2024

@potiuk @DjVinnii There was a conflict that was difficult to resolve and to do so would require us to include a PR which is part of AIP-49 and have a milestone of 2.10.

I can take a second look at it, but the conflict resolution won't be clean if we don't include the earlier PR. Please let me know if we still want to include it.

@potiuk
Copy link
Member

potiuk commented Jul 2, 2024

I think the change could be applied manually - this is a very simple change - we've done that in the past.

@utkarsharma2
Copy link
Contributor

I think the change could be applied manually - this is a very simple change - we've done that in the past.

Sure, I'll manually apply them.

@raphaelauv
Copy link
Contributor

@potiuk @ferruzzi

#34405 , is that not a concern ?

airflow that stop working cause otel-collector is down is not similar to what happen with statsd

@potiuk
Copy link
Member

potiuk commented Jul 3, 2024

#34405 , is that not a concern ?

airflow that stop working cause otel-collector is down is not similar to what happen with statsd

It's independent from being "experimental" - it's just regular - potential - bug fix that @feruzzi was supposed to follow up on :D

It does require discussion. I think the current behaviour (failure when otel collector is down) is correct. Mostly because if you are using OTEL collector, it becomes your way to monitor some issues and silently skipping over metrics generation is not likely a good idea (if you have permanent issue with your otel collector - you can still disable it in configuration until it is solved, with generally the same result as if we skipped it).

But I think it requires a short discussion and asking what others think, but it's not blocking the experimental status changing (also removal of the banner is not equal with removing experimental status - it's just that the banner was a bad idea in the first place).

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants