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.

Sign up for GitHub

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

Running mypy on sdk resources #773 #4360

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Running mypy on sdk resources #773 #4360

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3ab4dc7
Running mypy on sdk resources #773
prabhakarjuzgar Dec 16, 2024
c38154e
Update changelog
prabhakarjuzgar Dec 16, 2024
5c6d89b
Remove unicode chars and unwanted ignore
prabhakarjuzgar Dec 18, 2024
fc6c6e5
Remove unnecessary changes from CHANGELOG
prabhakarjuzgar Dec 18, 2024
822e841
Change importlib_metadata to importlib.metadata
prabhakarjuzgar Dec 20, 2024
192db0c
Address review comment
prabhakarjuzgar Dec 20, 2024
853c00f
Use importlib_metadata if importlib.data is unavailable
prabhakarjuzgar Dec 21, 2024
2ddbd55
Merge branch 'main' into pr773
xrmx Dec 23, 2024
d42a650
Remove unnecessary changes
prabhakarjuzgar Dec 23, 2024
99f1ff4
do not change implementation
prabhakarjuzgar Dec 23, 2024
825c727
do not change implementation final
prabhakarjuzgar Dec 23, 2024
ce4c3a7
suppress specific warnings
prabhakarjuzgar Dec 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 16 additions & 17 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## Unreleased

- Run mypy on SDK resources
prabhakarjuzgar marked this conversation as resolved.
Show resolved Hide resolved
([#4360](https://github.com/open-telemetry/opentelemetry-python/pull/4360))

## Version 1.29.0/0.50b0 (2024-12-11)

- Fix crash exporting a log record with None body
Expand Down Expand Up @@ -126,20 +129,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Improve resource field structure for LogRecords
([#3972](https://github.com/open-telemetry/opentelemetry-python/pull/3972))
- Update Semantic Conventions code generation scripts:
- fix namespace exclusion that resulted in dropping `os` and `net` namespaces.
- fix namespace exclusion that resulted in dropping `os` and `net` namespaces.
- add `Final` decorator to constants to prevent collisions
- enable mypy and fix detected issues
- allow to drop specific attributes in preparation for Semantic Conventions v1.26.0
([#3973](https://github.com/open-telemetry/opentelemetry-python/pull/3966))
([#3973](https://github.com/open-telemetry/opentelemetry-python/pull/3966))
- Update semantic conventions to version 1.26.0.
([#3964](https://github.com/open-telemetry/opentelemetry-python/pull/3964))
- Use semconv exception attributes for record exceptions in spans
([#3979](https://github.com/open-telemetry/opentelemetry-python/pull/3979))
- Fix _encode_events assumes events.attributes.dropped exists
- Fix \_encode_events assumes events.attributes.dropped exists
([#3965](https://github.com/open-telemetry/opentelemetry-python/pull/3965))
- Validate links at span creation
([#3991](https://github.com/open-telemetry/opentelemetry-python/pull/3991))
- Add attributes field in `MeterProvider.get_meter` and `InstrumentationScope`
- Add attributes field in `MeterProvider.get_meter` and `InstrumentationScope`
([#4015](https://github.com/open-telemetry/opentelemetry-python/pull/4015))
- Fix inaccessible `SCHEMA_URL` constants in `opentelemetry-semantic-conventions`
([#4069](https://github.com/open-telemetry/opentelemetry-python/pull/4069))
Expand Down Expand Up @@ -169,10 +172,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- `SpanAttributes`, `ResourceAttributes`, and `MetricInstruments` are deprecated.
- Attribute and metric definitions are now grouped by the namespace.
- Stable attributes and metrics are moved to `opentelemetry.semconv.attributes`
and `opentelemetry.semconv.metrics` modules.
and `opentelemetry.semconv.metrics` modules.
- Stable and experimental attributes and metrics are defined under
`opentelemetry.semconv._incubating` import path.
([#3586](https://github.com/open-telemetry/opentelemetry-python/pull/3586))
`opentelemetry.semconv._incubating` import path.
([#3586](https://github.com/open-telemetry/opentelemetry-python/pull/3586))
- Rename test objects to avoid pytest warnings
([#3823] (https://github.com/open-telemetry/opentelemetry-python/pull/3823))
- Add span flags to OTLP spans and links
Expand All @@ -184,11 +187,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Fix prometheus metric name and unit conversion
([#3924](https://github.com/open-telemetry/opentelemetry-python/pull/3924))
- this is a breaking change to prometheus metric names so they comply with the
[specification](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.33.0/specification/compatibility/prometheus_and_openmetrics.md#otlp-metric-points-to-prometheus).
[specification](https://github.com/open-telemetry/opentelemetry-specification/blob/v1.33.0/specification/compatibility/prometheus_and_openmetrics.md#otlp-metric-points-to-prometheus).
- you can temporarily opt-out of the unit normalization by setting the environment variable
`OTEL_PYTHON_EXPERIMENTAL_DISABLE_PROMETHEUS_UNIT_NORMALIZATION=true`
`OTEL_PYTHON_EXPERIMENTAL_DISABLE_PROMETHEUS_UNIT_NORMALIZATION=true`
- common unit abbreviations are converted to Prometheus conventions (`s` -> `seconds`),
following the [collector's implementation](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/c0b51136575aa7ba89326d18edb4549e7e1bbdb9/pkg/translator/prometheus/normalize_name.go#L108)
following the [collector's implementation](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/c0b51136575aa7ba89326d18edb4549e7e1bbdb9/pkg/translator/prometheus/normalize_name.go#L108)
- repeated `_` are replaced with a single `_`
- unit annotations (enclosed in curly braces like `{requests}`) are stripped away
- units with slash are converted e.g. `m/s` -> `meters_per_second`.
Expand Down Expand Up @@ -274,7 +277,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## Version 1.21.0/0.42b0 (2023-11-01)

- Fix `SumAggregation`
 ([#3390](https://github.com/open-telemetry/opentelemetry-python/pull/3390))
 ([#3390](https://github.com/open-telemetry/opentelemetry-python/pull/3390))
prabhakarjuzgar marked this conversation as resolved.
Show resolved Hide resolved
- Fix handling of empty metric collection cycles
([#3335](https://github.com/open-telemetry/opentelemetry-python/pull/3335))
- Fix error when no LoggerProvider configured for LoggingHandler
Expand All @@ -294,7 +297,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Implement Process Resource detector
([#3472](https://github.com/open-telemetry/opentelemetry-python/pull/3472))


## Version 1.20.0/0.41b0 (2023-09-04)

- Modify Prometheus exporter to translate non-monotonic Sums into Gauges
Expand Down Expand Up @@ -323,7 +325,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Default LogRecord observed_timestamp to current timestamp
[#3377](https://github.com/open-telemetry/opentelemetry-python/pull/3377))


## Version 1.18.0/0.39b0 (2023-05-19)

- Select histogram aggregation with an environment variable
Expand All @@ -343,7 +344,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Add benchmark tests for metrics
([#3267](https://github.com/open-telemetry/opentelemetry-python/pull/3267))


## Version 1.17.0/0.38b0 (2023-03-22)

- Implement LowMemory temporality
Expand All @@ -363,7 +363,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## Version 1.16.0/0.37b0 (2023-02-17)

- Change ``__all__`` to be statically defined.
- Change `__all__` to be statically defined.
([#3143](https://github.com/open-telemetry/opentelemetry-python/pull/3143))
- Remove the ability to set a global metric prefix for Prometheus exporter
([#3137](https://github.com/open-telemetry/opentelemetry-python/pull/3137))
Expand Down Expand Up @@ -403,7 +403,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#3027](https://github.com/open-telemetry/opentelemetry-python/pull/3027))
- Update logging to include logging api as per specification
([#3038](https://github.com/open-telemetry/opentelemetry-python/pull/3038))
- Fix: Avoid generator in metrics _ViewInstrumentMatch.collect()
- Fix: Avoid generator in metrics \_ViewInstrumentMatch.collect()
([#3035](https://github.com/open-telemetry/opentelemetry-python/pull/3035)
- [exporter-otlp-proto-grpc] add user agent string
([#3009](https://github.com/open-telemetry/opentelemetry-python/pull/3009))
Expand Down Expand Up @@ -1691,7 +1691,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Remove dependency on 'backoff' library
([#3679](https://github.com/open-telemetry/opentelemetry-python/pull/3679))


- Make create_gauge non-abstract method
([#3817](https://github.com/open-telemetry/opentelemetry-python/pull/3817))
- Make `tracer.start_as_current_span()` decorator work with async functions
Expand Down
27 changes: 14 additions & 13 deletions opentelemetry-sdk/src/opentelemetry/sdk/error_handler/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ def _handle(self, error: Exception, *args, **kwargs):

from abc import ABC, abstractmethod
from logging import getLogger
from typing import Optional
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather prefer to use from __future__ import annotations on the top of the imports, and use pipe (|) annotation.


from opentelemetry.util._importlib_metadata import entry_points

Expand All @@ -69,7 +70,7 @@ def _handle(self, error: Exception, *args, **kwargs):

class ErrorHandler(ABC):
@abstractmethod
def _handle(self, error: Exception, *args, **kwargs):
def _handle(self, error: Exception, *args, **kwargs) -> None: # type: ignore
"""
Handle an exception
"""
Expand All @@ -83,7 +84,7 @@ class _DefaultErrorHandler(ErrorHandler):
"""

# pylint: disable=useless-return
def _handle(self, error: Exception, *args, **kwargs):
def _handle(self, error: Exception, *args, **kwargs) -> None: # type: ignore
logger.exception("Error handled by default error handler: ")
return None

Expand All @@ -105,26 +106,26 @@ def __new__(cls) -> "GlobalErrorHandler":

return cls._instance

def __enter__(self):
def __enter__(self) -> None:
pass

# pylint: disable=no-self-use
def __exit__(self, exc_type, exc_value, traceback):
if exc_value is None:
def __exit__(self, exc_type, exc_value, traceback) -> Optional[bool]: # type: ignore
if exc_value is None: # 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.

If above applied, the ignore is not needed.

Suggested change
if exc_value is None: # type: ignore
if exc_value is None:

return None

plugin_handled = False

error_handler_entry_points = entry_points(
error_handler_entry_points = entry_points( # 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.

This can be typed, https://github.com/python/importlib_metadata/blob/main/importlib_metadata/__init__.py#L1039C42-L1039C43
Need to check if we are exporting it from our util module

Copy link
Author

Choose a reason for hiding this comment

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

It is - https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/src/opentelemetry/util/_importlib_metadata.py#L28

Copy link
Author

Choose a reason for hiding this comment

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

basedpyright: Import "opentelemetry.util._importlib_metadata" could not be resolved. This results in following errors when mypy is executed -

opentelemetry-sdk/src/opentelemetry/sdk/error_handler/init.py:119: error: Type of variable becomes "Any" due to an unfollowed import [no-any-unimported]
opentelemetry-sdk/src/opentelemetry/sdk/error_handler/init.py:119: error: Expression has type "Any" [misc]

Copy link
Author

Choose a reason for hiding this comment

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

@xrmx Please let me know if the above errors can be handled differently.

Copy link
Contributor

@xrmx xrmx Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I mean something like this error_handler_entry_points : Entrypoints = entry_points(

Copy link
Contributor

Choose a reason for hiding this comment

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

This PR has more type ignores than actual typing improvements.

Copy link
Author

Choose a reason for hiding this comment

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

This is a tricky one to type hint because of the way https://github.com/open-telemetry/opentelemetry-python/blob/main/opentelemetry-api/src/opentelemetry/util/_importlib_metadata.py is implemented.

Copy link
Author

Choose a reason for hiding this comment

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

@Kludex do you have any suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

This PR is really only adding type: ignore, there isn't any benefit to do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion is for the project to fix the mypy setup in this project, or move to pyright.

I'm happy to help, if the maintainers are willing.

If you can help, please create an issue about the mypy setup.

group="opentelemetry_error_handler"
)

for error_handler_entry_point in error_handler_entry_points:
error_handler_class = error_handler_entry_point.load()
for error_handler_entry_point in error_handler_entry_points: # type: ignore
error_handler_class = error_handler_entry_point.load() # type: ignore

if issubclass(error_handler_class, exc_value.__class__):
if issubclass(error_handler_class, exc_value.__class__): # type: ignore
try:
error_handler_class()._handle(exc_value)
error_handler_class()._handle(exc_value) # type: ignore
plugin_handled = True

# pylint: disable=broad-exception-caught
Expand All @@ -133,11 +134,11 @@ def __exit__(self, exc_type, exc_value, traceback):
"%s error while handling error"
" %s by error handler %s",
error_handling_error.__class__.__name__,
exc_value.__class__.__name__,
error_handler_class.__name__,
exc_value.__class__.__name__, # type: ignore
error_handler_class.__name__, # type: ignore
)

if not plugin_handled:
_DefaultErrorHandler()._handle(exc_value)
_DefaultErrorHandler()._handle(exc_value) # type: ignore

return True
16 changes: 8 additions & 8 deletions opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/exemplar/exemplar_reservoir.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ def collect(self, point_attributes: Attributes) -> Optional[Exemplar]:
{
k: v
for k, v in self.__attributes.items()
if k not in point_attributes
if k not in point_attributes # type: ignore
}
if self.__attributes
else None
Expand Down Expand Up @@ -162,8 +162,8 @@ class BucketIndexError(ValueError):
class FixedSizeExemplarReservoirABC(ExemplarReservoir):
"""Abstract class for a reservoir with fixed size."""

def __init__(self, size: int, **kwargs) -> None:
super().__init__(**kwargs)
def __init__(self, size: int, **kwargs) -> None: # type: ignore
super().__init__(**kwargs) # type: ignore
self._size: int = size
self._reservoir_storage: Mapping[int, ExemplarBucket] = defaultdict(
ExemplarBucket
Expand All @@ -184,7 +184,7 @@ def collect(self, point_attributes: Attributes) -> List[Exemplar]:
exemplars = [
e
for e in (
bucket.collect(point_attributes)
bucket.collect(point_attributes) # type: ignore
prabhakarjuzgar marked this conversation as resolved.
Show resolved Hide resolved
for _, bucket in sorted(self._reservoir_storage.items())
)
if e is not None
Expand Down Expand Up @@ -257,8 +257,8 @@ class SimpleFixedSizeExemplarReservoir(FixedSizeExemplarReservoirABC):
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#simplefixedsizeexemplarreservoir
"""

def __init__(self, size: int = 1, **kwargs) -> None:
super().__init__(size, **kwargs)
def __init__(self, size: int = 1, **kwargs) -> None: # type: ignore
super().__init__(size, **kwargs) # type: ignore
self._measurements_seen: int = 0

def _reset(self) -> None:
Expand Down Expand Up @@ -292,8 +292,8 @@ class AlignedHistogramBucketExemplarReservoir(FixedSizeExemplarReservoirABC):
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#alignedhistogrambucketexemplarreservoir
"""

def __init__(self, boundaries: Sequence[float], **kwargs) -> None:
super().__init__(len(boundaries) + 1, **kwargs)
def __init__(self, boundaries: Sequence[float], **kwargs) -> None: # type: ignore
super().__init__(len(boundaries) + 1, **kwargs) # type: ignore
self._boundaries: Sequence[float] = boundaries

def offer(
Expand Down