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

otelgrpc: Add metrics tests for ServerStreamInterceptors #4323

Closed
MadVikingGod opened this issue Sep 21, 2023 · 4 comments
Closed

otelgrpc: Add metrics tests for ServerStreamInterceptors #4323

MadVikingGod opened this issue Sep 21, 2023 · 4 comments
Assignees
Labels
area: instrumentation Related to an instrumentation package enhancement New feature or request instrumentation: otelgrpc

Comments

@MadVikingGod
Copy link
Contributor

Problem Statement

There are two problems discovered in #4322:

  1. There are no tests around the metrics produced by a ServerStreamInteceptor
  2. Under test, ServerStreamInteceptors don't have peerInfo within the call made. This means that peer info will not show in tests, but does show in actual use.

Proposed Solution

Write a test similar to checkUnaryServerRecords to assert that the produced metrics match

Change the test such that it does have peerInfo within it's context

@MadVikingGod MadVikingGod added enhancement New feature or request area: instrumentation Related to an instrumentation package instrumentation: otelgrpc labels Sep 21, 2023
@pellared pellared changed the title Add metrics tests for ServerStreamInterceptors otelgrpc: Add metrics tests for ServerStreamInterceptors Sep 21, 2023
@pellared pellared added this to the v0.45.0 milestone Sep 21, 2023
@fatsheep9146
Copy link
Contributor

fatsheep9146 commented Sep 26, 2023

@pellared @MadVikingGod

func StreamServerInterceptor(opts ...Option) grpc.StreamServerInterceptor {
cfg := newConfig(opts)
tracer := cfg.TracerProvider.Tracer(
instrumentationName,
trace.WithInstrumentationVersion(Version()),
)
return func(
srv interface{},
ss grpc.ServerStream,
info *grpc.StreamServerInfo,
handler grpc.StreamHandler,
) error {
ctx := ss.Context()
i := &InterceptorInfo{
StreamServerInfo: info,
Type: StreamServer,
}
if cfg.Filter != nil && !cfg.Filter(i) {
return handler(srv, wrapServerStream(ctx, ss, cfg))
}
ctx = extract(ctx, cfg.Propagators)
name, attr := spanInfo(info.FullMethod, peerFromCtx(ctx))
startOpts := append([]trace.SpanStartOption{
trace.WithSpanKind(trace.SpanKindServer),
trace.WithAttributes(attr...)},
cfg.SpanStartOptions...,
)
ctx, span := tracer.Start(
trace.ContextWithRemoteSpanContext(ctx, trace.SpanContextFromContext(ctx)),
name,
startOpts...,
)
defer span.End()
err := handler(srv, wrapServerStream(ctx, ss, cfg))
if err != nil {
s, _ := status.FromError(err)
statusCode, msg := serverStatus(s)
span.SetStatus(statusCode, msg)
span.SetAttributes(statusCodeAttr(s.Code()))
} else {
span.SetAttributes(statusCodeAttr(grpc_codes.OK))
}
return err
}
}

I found that the ServerStreamInterceptors do not produce metric for now, only ServerUnaryInterceptors support, do we need add metrics support for ServerStreamInterceptors and then add test for it? Or should we directly support metrics in StatsHandler, since interceptors will be deprecated in some day.

@pellared
Copy link
Member

pellared commented Sep 26, 2023

I think that we wanted to add tests for metrics emitted by server interceptors.

I propose to leave implementation and just add assertions for metrics emitted by the interceptors. You can consider adding an assertion showing that ServerStreamInterceptors does NOT produce any metrics.

I think adding support for metrics in ServerStreamInterceptors is a waste of our time. I would rather work towards making StatsHandler as good as possible.

@MadVikingGod do you agree?

@MadVikingGod
Copy link
Contributor Author

I was under the impression that the different Interceptors were being deprecated, if that's the case, then I don't think any functionality should be added.

I think there should be some testing around what metrics (if any) are put out by both the stat's handler and the interceptors, and peer Attributes must be part of that dataset.

@MrAlias MrAlias modified the milestones: v1.20.0/v0.45.0, v0.46.0 Sep 28, 2023
@pellared pellared modified the milestones: v0.46.0, v0.47.0 Nov 6, 2023
@pellared
Copy link
Member

pellared commented Nov 7, 2023

Closing per #4318

@pellared pellared closed this as not planned Won't fix, can't repro, duplicate, stale Nov 7, 2023
@pellared pellared removed this from the v0.47.0 milestone Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: instrumentation Related to an instrumentation package enhancement New feature or request instrumentation: otelgrpc
Projects
None yet
Development

No branches or pull requests

4 participants