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

Add logfire.metric_gauge() #153

Merged
merged 7 commits into from
May 8, 2024
Merged

Add logfire.metric_gauge() #153

merged 7 commits into from
May 8, 2024

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented May 8, 2024

Checklist

  • Add tests.
  • Add documentation.
    • Improve the explanation on "why" to use each metric, and give examples.

@Kludex Kludex changed the title Add metric_gauge Add logfire.metric_gauge() May 8, 2024
Copy link

cloudflare-workers-and-pages bot commented May 8, 2024

Deploying logfire-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8043ffe
Status: ✅  Deploy successful!
Preview URL: https://dae51337.logfire-docs.pages.dev
Branch Preview URL: https://add-gauge-metric.logfire-docs.pages.dev

View logs

Copy link

codecov bot commented May 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@Kludex Kludex marked this pull request as ready for review May 8, 2024 11:32
except ImportError: # pragma: no cover
Gauge = None
GAUGE_IMPORTED = False # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why this is better than Gauge = None, and it requires 2 extra type-ignores.

Copy link
Member Author

@Kludex Kludex May 8, 2024

Choose a reason for hiding this comment

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

It actually avoids the type ignore on the type hint of the metric_gauge method - but I just had an idea on how to avoid one more.

I'll eat and come back to this.

def create_gauge(
self,
name: str,
unit: str = '',
description: str = '',
): # pragma: no cover
) -> _Gauge: # pragma: no cover
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the problem.

@Kludex Kludex requested a review from alexmojaki May 8, 2024 13:54
@@ -353,7 +361,7 @@ def _create_real_instrument(self, meter: Meter) -> UpDownCounter:
return meter.create_up_down_counter(self._name, self._unit, self._description)


if Gauge is not None: # pragma: no branch
if Gauge is not None: # pragma: no cover
Copy link
Contributor

Choose a reason for hiding this comment

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

Surely this is covered?

Copy link
Member Author

Choose a reason for hiding this comment

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

My bad, I was just trying to readd what was previously there.

Comment on lines +27 to +29
from opentelemetry.metrics import _Gauge

Gauge = _Gauge
Copy link
Contributor

Choose a reason for hiding this comment

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

interesting, pyright treats this differently to import _Gauge as Gauge?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did this because we need a return type for create_gauge in this file, and without this line, Gauge can either be _Gauge or None. Since it can be either, then the type of create_gauge is wrong.

That's why the use of another variable - the type checker will know that Gauge is _Gauge.

tests/test_metrics.py Outdated Show resolved Hide resolved
Co-authored-by: Alex Hall <alex.mojaki@gmail.com>
logfire/_internal/metrics.py Outdated Show resolved Hide resolved
@Kludex Kludex requested a review from alexmojaki May 8, 2024 14:12
@Kludex Kludex merged commit efe015e into main May 8, 2024
13 checks passed
@Kludex Kludex deleted the add-gauge-metric branch May 8, 2024 14:33
@adriangb
Copy link
Member

adriangb commented May 8, 2024

Thanks I needed this the other day 😆

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.

Generate metric points from pydantic class changes?
3 participants