Skip to content

Checked if instrument enabled before measuring in oteltracegrpc #7825

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

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

Checked if instrument enabled before measuring in oteltracegrpc #7825

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e5cbcd8
checked if fields enabled before adding to metrics
itssaharsh Jan 22, 2026
21dea04
Update exporters/otlp/otlptrace/otlptracegrpc/internal/observ/instrum…
itssaharsh Jan 23, 2026
9fd1289
Update exporters/otlp/otlptrace/otlptracegrpc/internal/observ/instrum…
itssaharsh Jan 23, 2026
a381257
lint fix annd logic fix
itssaharsh Jan 23, 2026
0a77a74
Merge branch 'main' into bump
itssaharsh Jan 25, 2026
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
79 changes: 45 additions & 34 deletions exporters/otlp/otlptrace/otlptracegrpc/internal/observ/instrumentation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,9 @@ var (
}
)

func get[T any](p *sync.Pool) *[]T { return p.Get().(*[]T) }
func get[T any](p *sync.Pool) *[]T {
return p.Get().(*[]T)
}

func put[T any](p *sync.Pool, s *[]T) {
*s = (*s)[:0] // Reset.
Expand Down Expand Up @@ -205,11 +207,12 @@ func BaseAttrs(id int64, target string) []attribute.KeyValue {
// ExportSpans method returns.
func (i *Instrumentation) ExportSpans(ctx context.Context, nSpans int) ExportOp {
start := time.Now()

addOpt := get[metric.AddOption](addOptPool)
defer put(addOptPool, addOpt)
*addOpt = append(*addOpt, i.addOpt)
i.inflightSpans.Add(ctx, int64(nSpans), *addOpt...)
if i.inflightSpans.Enabled(ctx) {
addOpt := get[metric.AddOption](addOptPool)
defer put(addOptPool, addOpt)
*addOpt = append(*addOpt, i.addOpt)
i.inflightSpans.Add(ctx, int64(nSpans), *addOpt...)
}

return ExportOp{
ctx: ctx,
Expand Down Expand Up @@ -238,38 +241,46 @@ type ExportOp struct {
// of successfully exported spans will be determined by inspecting the
// RejectedItems field of the PartialSuccess.
func (e ExportOp) End(err error, code codes.Code) {
addOpt := get[metric.AddOption](addOptPool)
defer put(addOptPool, addOpt)
*addOpt = append(*addOpt, e.inst.addOpt)

e.inst.inflightSpans.Add(e.ctx, -e.nSpans, *addOpt...)

success := successful(e.nSpans, err)
// Record successfully exported spans, even if the value is 0 which are
// meaningful to distribution aggregations.
e.inst.exportedSpans.Add(e.ctx, success, *addOpt...)
isOn := e.inst.inflightSpans.Enabled(e.ctx)
esOn := e.inst.exportedSpans.Enabled(e.ctx)
if isOn || esOn {
success := successful(e.nSpans, err)
addOpt := get[metric.AddOption](addOptPool)
defer put(addOptPool, addOpt)
*addOpt = append(*addOpt, e.inst.addOpt)
Comment on lines +247 to +250
Copy link
Contributor

@MrAlias MrAlias Jan 22, 2026

Choose a reason for hiding this comment

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

Preserve the existing code flow. Changing this is not needed and a distraction for this PR.

Copy link
Contributor Author

@itssaharsh itssaharsh Jan 23, 2026

Choose a reason for hiding this comment

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

Hey so I got stuck here because if the if block in which success is there doesn't run the other block where success is used will panick that's why I changed structure.

if isOn {
e.inst.inflightSpans.Add(e.ctx, -e.nSpans, *addOpt...)
}
// Record successfully exported spans, even if the value is 0 which are
// meaningful to distribution aggregations.
if esOn {
e.inst.exportedSpans.Add(e.ctx, success, *addOpt...)
if err != nil {
attrs := get[attribute.KeyValue](measureAttrsPool)
defer put(measureAttrsPool, attrs)
*attrs = append(*attrs, e.inst.attrs...)
*attrs = append(*attrs, semconv.ErrorType(err))

// Do not inefficiently make a copy of attrs by using
// WithAttributes instead of WithAttributeSet.
o := metric.WithAttributeSet(attribute.NewSet(*attrs...))
// Reset addOpt with new attribute set.
*addOpt = append((*addOpt)[:0], o)

e.inst.exportedSpans.Add(e.ctx, e.nSpans-success, *addOpt...)
}
}
}

if err != nil {
attrs := get[attribute.KeyValue](measureAttrsPool)
defer put(measureAttrsPool, attrs)
*attrs = append(*attrs, e.inst.attrs...)
*attrs = append(*attrs, semconv.ErrorType(err))
if e.inst.opDuration.Enabled(e.ctx) {
recOpt := get[metric.RecordOption](recordOptPool)
*recOpt = append(*recOpt, e.inst.recordOption(err, code))

// Do not inefficiently make a copy of attrs by using
// WithAttributes instead of WithAttributeSet.
o := metric.WithAttributeSet(attribute.NewSet(*attrs...))
// Reset addOpt with new attribute set.
*addOpt = append((*addOpt)[:0], o)
d := time.Since(e.start).Seconds()
e.inst.opDuration.Record(e.ctx, d, *recOpt...)

e.inst.exportedSpans.Add(e.ctx, e.nSpans-success, *addOpt...)
put(recordOptPool, recOpt)
}

recOpt := get[metric.RecordOption](recordOptPool)
defer put(recordOptPool, recOpt)
Copy link
Contributor

@MrAlias MrAlias Jan 22, 2026

Choose a reason for hiding this comment

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

Why is this changed?

Copy link
Contributor Author

@itssaharsh itssaharsh Jan 23, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I changed it because I wanted to put back the slice manually each time. I will change it to defer if it is better but I thought putting it back instantly would be better.

*recOpt = append(*recOpt, e.inst.recordOption(err, code))

d := time.Since(e.start).Seconds()
e.inst.opDuration.Record(e.ctx, d, *recOpt...)
}

// recordOption returns a RecordOption with attributes representing the
Expand Down
Loading