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

prometheus: Add instrumentation scope attributes to otel_scope_info #5932

Merged
merged 8 commits into from
Nov 5, 2024

Conversation

pellared
Copy link
Member

@pellared pellared commented Oct 30, 2024

Towards #5846

Pre-work: #5806

Copy link

codecov bot commented Oct 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.6%. Comparing base (1230566) to head (9cee213).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #5932   +/-   ##
=====================================
  Coverage   84.6%   84.6%           
=====================================
  Files        272     272           
  Lines      22858   22864    +6     
=====================================
+ Hits       19341   19348    +7     
+ Misses      3173    3172    -1     
  Partials     344     344           

see 2 files with indirect coverage changes

@pellared pellared changed the title Prometheus exporter exports instrumentation scope attributes prometheus: Export instrumentation scope attributes in otel_scope_info Oct 30, 2024
@pellared pellared changed the title prometheus: Export instrumentation scope attributes in otel_scope_info prometheus: Add instrumentation scope attributes to otel_scope_info Oct 30, 2024
@pellared pellared marked this pull request as ready for review October 30, 2024 11:51
@pellared pellared added this to the v1.32.0 milestone Oct 30, 2024
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

pellared added a commit that referenced this pull request Nov 4, 2024
Addresses
#5932 (comment)

I feel that `getAttr` was doing too much and having:

```go
keys, values := getAttrs(dp.Attributes)
keys = append(keys, kv.keys...)
values = append(values, kv.vals...)
// ...
keys, values := getAttrs(*res.Set())
```

is more readable than:
```go
keys, values := getAttrs(dp.Attributes, ks, vs, resourceKV)
// ...
keys, values := getAttrs(*res.Set(), [2]string{}, [2]string{}, keyVals{})
```

Benchmarks results just in case to minimize the possibility of
accidental introduction of a performance overhead:

```
$ benchstat old.txt new.txt 
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/exporters/prometheus
cpu: Intel(R) Core(TM) i9-10885H CPU @ 2.40GHz
                │   old.txt    │               new.txt               │
                │    sec/op    │    sec/op     vs base               │
Collect1-16       29.77µ ± 11%   30.33µ ± 10%       ~ (p=0.529 n=10)
Collect10-16      80.58µ ±  7%   77.93µ ± 15%       ~ (p=0.315 n=10)
Collect100-16     528.5µ ±  7%   511.2µ ±  2%  -3.28% (p=0.015 n=10)
Collect1000-16    3.179m ±  6%   3.344m ± 15%  +5.19% (p=0.003 n=10)
Collect10000-16   31.77m ±  2%   33.14m ±  7%  +4.34% (p=0.004 n=10)
geomean           662.9µ         668.9µ        +0.90%

                │   old.txt    │               new.txt               │
                │     B/op     │     B/op      vs base               │
Collect1-16       36.52Ki ± 0%   36.59Ki ± 0%  +0.17% (p=0.000 n=10)
Collect10-16      64.58Ki ± 0%   64.64Ki ± 0%  +0.09% (p=0.000 n=10)
Collect100-16     349.3Ki ± 0%   349.4Ki ± 0%  +0.03% (p=0.000 n=10)
Collect1000-16    3.163Mi ± 0%   3.163Mi ± 0%       ~ (p=0.247 n=10)
Collect10000-16   31.05Mi ± 0%   31.06Mi ± 0%  +0.02% (p=0.009 n=10)
geomean           610.6Ki        611.0Ki       +0.06%

                │   old.txt   │              new.txt               │
                │  allocs/op  │  allocs/op   vs base               │
Collect1-16        70.00 ± 0%    72.00 ± 0%  +2.86% (p=0.000 n=10)
Collect10-16       396.0 ± 0%    398.0 ± 0%  +0.51% (p=0.000 n=10)
Collect100-16     3.661k ± 0%   3.663k ± 0%  +0.05% (p=0.000 n=10)
Collect1000-16    36.15k ± 0%   36.15k ± 0%  +0.01% (p=0.000 n=10)
Collect10000-16   361.4k ± 0%   361.5k ± 0%  +0.03% (p=0.009 n=10)
geomean           4.212k        4.241k       +0.68%
```
@pellared
Copy link
Member Author

pellared commented Nov 4, 2024

SIG meeting:
Discussed with @dashpole. There would be a follow-up (seperate) PR to make scope attributes and scheme URL as identifying. See open-telemetry/opentelemetry-specification#4223 (comment). These changes are not conflicting.

@pellared
Copy link
Member Author

pellared commented Nov 5, 2024

@dashpole, merging to avoid conflicts when working on "make scope attributes and scheme URL as identifying". I can still address any comments that will be added later (post-merge).

@pellared pellared merged commit ff07838 into open-telemetry:main Nov 5, 2024
31 of 32 checks passed
@pellared pellared deleted the prom-scope-attrs branch November 5, 2024 14:04
pellared added a commit that referenced this pull request Nov 8, 2024
### Added

- Add `go.opentelemetry.io/otel/sdk/metric/exemplar.AlwaysOffFilter`,
which can be used to disable exemplar recording. (#5850)
- Add `go.opentelemetry.io/otel/sdk/metric.WithExemplarFilter`, which
can be used to configure the exemplar filter used by the metrics SDK.
(#5850)
- Add `ExemplarReservoirProviderSelector` and
`DefaultExemplarReservoirProviderSelector` to
`go.opentelemetry.io/otel/sdk/metric`, which defines the exemplar
reservoir to use based on the aggregation of the metric. (#5861)
- Add `ExemplarReservoirProviderSelector` to
`go.opentelemetry.io/otel/sdk/metric.Stream` to allow using views to
configure the exemplar reservoir to use for a metric. (#5861)
- Add `ReservoirProvider`, `HistogramReservoirProvider` and
`FixedSizeReservoirProvider` to
`go.opentelemetry.io/otel/sdk/metric/exemplar` to make it convenient to
use providers of Reservoirs. (#5861)
- The `go.opentelemetry.io/otel/semconv/v1.27.0` package.
The package contains semantic conventions from the `v1.27.0` version of
the OpenTelemetry Semantic Conventions. (#5894)
- Add `Attributes attribute.Set` field to `Scope` in
`go.opentelemetry.io/otel/sdk/instrumentation`. (#5903)
- Add `Attributes attribute.Set` field to `ScopeRecords` in
`go.opentelemetry.io/otel/log/logtest`. (#5927)
- `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc` adds
instrumentation scope attributes. (#5934)
- `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp` adds
instrumentation scope attributes. (#5934)
- `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc`
adds instrumentation scope attributes. (#5935)
- `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp`
adds instrumentation scope attributes. (#5935)
- `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc` adds
instrumentation scope attributes. (#5933)
- `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploghttp` adds
instrumentation scope attributes. (#5933)
- `go.opentelemetry.io/otel/exporters/prometheus` adds instrumentation
scope attributes in `otel_scope_info` metric as labels. (#5932)

### Changed

- Support scope attributes and make them as identifying for `Tracer` in
`go.opentelemetry.io/otel` and `go.opentelemetry.io/otel/sdk/trace`.
(#5924)
- Support scope attributes and make them as identifying for `Meter` in
`go.opentelemetry.io/otel` and `go.opentelemetry.io/otel/sdk/metric`.
(#5926)
- Support scope attributes and make them as identifying for `Logger` in
`go.opentelemetry.io/otel` and `go.opentelemetry.io/otel/sdk/log`.
(#5925)
- Make schema URL and scope attributes as identifying for `Tracer` in
`go.opentelemetry.io/otel/bridge/opentracing`. (#5931)
- Clear unneeded slice elements to allow GC to collect the objects in
`go.opentelemetry.io/otel/sdk/metric` and
`go.opentelemetry.io/otel/sdk/trace`. (#5804)

### Fixed

- Global MeterProvider registration unwraps global instrument Observers,
the undocumented Unwrap() methods are now private. (#5881)
- `go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc`
now keeps the metadata already present in the context when `WithHeaders`
is used. (#5892)
- `go.opentelemetry.io/otel/exporters/otlp/otlplog/otlploggrpc` now
keeps the metadata already present in the context when `WithHeaders` is
used. (#5911)
- `go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc` now
keeps the metadata already present in the context when `WithHeaders` is
used. (#5915)
- Fix `go.opentelemetry.io/otel/exporters/prometheus` trying to add
exemplars to Gauge metrics, which is unsupported. (#5912)
- Fix `WithEndpointURL` to always use a secure connection when an https
URL is passed in
`go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc`.
(#5944)
- Fix `WithEndpointURL` to always use a secure connection when an https
URL is passed in
`go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp`.
(#5944)
- Fix `WithEndpointURL` to always use a secure connection when an https
URL is passed in
`go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc`.
(#5944)
- Fix `WithEndpointURL` to always use a secure connection when an https
URL is passed in
`go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp`.
(#5944)
- Fix incorrect metrics generated from callbacks when multiple readers
are used in `go.opentelemetry.io/otel/sdk/metric`. (#5900)

### Removed

- Remove all examples under `go.opentelemetry.io/otel/example` as they
are moved to [Contrib
repository](https://github.com/open-telemetry/opentelemetry-go-contrib/tree/main/examples).
(#5930)
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.

3 participants