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

Jump to bottom

GrpcMeterIdPrefixFunction? #2762

Closed
trustin opened this issue Jun 2, 2020 · 10 comments · Fixed by #2971
Closed

GrpcMeterIdPrefixFunction? #2762

trustin opened this issue Jun 2, 2020 · 10 comments · Fixed by #2971
Labels
new feature
Milestone
1.0.0

Comments

@trustin
Copy link
Member

trustin commented Jun 2, 2020

Originally suggested by @okue

It'd be nice to have a dedicated MeterIdPrefixFunction implementation for gRPC, so that the tag for grpc-status is added automatically.
@okue
Copy link
Member

okue commented Jun 15, 2020

To make MeterIdPrefixFunction easier to customize,
how about adding some functions to MeterIdPrefixFunction?

static MeterIdPrefixFunction ofDefault(
    String name,
    BiConsumer<MeterRegistry, ? super RequestOnlyLog> consumer
) {
    ...
}

or

default MeterIdPrefixFunction andThen(
    Function3<MeterRegistry, ? super RequestOnlyLog, MeterIdPrefix, MeterIdPrefix> consumer
) {
    ...
}

@trustin
Copy link
Member Author

trustin commented Jun 16, 2020

That's a good idea. I like the latter. What do you think, @line/dx?

@ikhoon
Copy link
Contributor

ikhoon commented Jun 17, 2020

I also prefer the andThen version. But Java does not have Function3 or triple function. We might need a customizer interface.

@okue okue mentioned this issue Jun 25, 2020
Merged
trustin pushed a commit that referenced this issue Jul 1, 2020
@okue
Add MeterIdPrefixFunction.andThen to customize MeterIdPrefix using Re…
66b7e7c 
…questLog (#2839)

Preparation for #2762.

Motivations:
Currently, `MeterIdPrefixFunction` can be customized with [`andThen`](https://github.com/line/armeria/blob/armeria-0.99.7/core/src/main/java/com/linecorp/armeria/common/metric/MeterIdPrefixFunction.java#L179-L193) which takes a function of type `(MeterRegistry, MeterIdPrefix) -> MeterIdPrefix`.
But, without taking `RequestLog`, we cannot append header values to `MeterIdPrefix`.

Modifications:
- Add `andThen: (MeterRegistry, RequestOnlyLog, MeterIdPrefix) -> MeterIdPrefix`
- Deprecate the original version `andThen(BiFunction<MeterRegistry, MeterIdPrefix, MeterIdPrefix>)`

Results:
Users can customize `MeterIdPrefixFunction` easily like

```java
MeterIdPrefixFunction
        .ofDefault("hoge")
        .andThen((registry, log, id) -> {
            if (log instanceof RequestLog) {
                return id.withTags("grpc-status", log.responseHeaders().get("grpc-status"));
            } else {
                return id;
            }
        });
```
@okue
Copy link
Member

okue commented Jul 1, 2020

Now, we can set grpc-status to MeterIdPrefix easily using MeterIdPrefixFunction#andThen(MeterIdPrefixFunctionCustomizer).

BTW, is it better to offer GrpcMeterIdPrefixFunction?
If so, should GrpcMeterIdPrefixFunction use andThen, or optimize something..?

@trustin
Copy link
Member Author

trustin commented Jul 1, 2020

That's a good question. I think it will be useful to provide one out of the box. It'd be nice if we have an optimized version, but we will have to refactor a little bit to de-duplicate the common logic from the default prefix function. Alternatively, we could duplicate a little bit and just write some tight test cases.

@ikhoon
Copy link
Contributor

ikhoon commented Jul 1, 2020

I prefer GrpcMeterIdPrefixFunction because grpc-status can be gotten from:

  • headers if a response body is empty
  • trailers if a response body is not empty and the protocol is normal gRPC
  • last of bodies if a response body is not empty and the protocol is gRPC-Web

@trustin
Copy link
Member Author

trustin commented Jul 22, 2020

How could we get the grpc-status trailer from a gRPC-Web response?

@trustin trustin added this to the 1.0.0 milestone Jul 22, 2020
@trustin
Copy link
Member Author

trustin commented Jul 22, 2020

Probably worth getting this done before 1.0 to solve the gRPC-Web trailer handling problem..

@minwoox
Copy link
Member

minwoox commented Jul 22, 2020

Let me work on this after finishing #2911

minwoox added a commit to minwoox/armeria that referenced this issue Aug 6, 2020
@minwoox
Add GrpcMeterIdPrefixFunction
3a0f60a 
Motivation:
It'd be nice to have a dedicated `MeterIdPrefixFunction` implementation for gRPC,
so that the tag for grpc-status is added automatically.

Modifications:
- Add `GrpcMeterIdPrefixFunction`.
- Insert `GrpcWebTrailersExtrator` before `HttpClientDelegate` when the gRPC-web client is created.
- Remove `GrpcWebUtil.parseTrailers()` which was used to extract web trailers from RetringClient.
  - Could use `GrpcWebUtil.trailers(ctx)` to get gRPC web trailers instead.
- Add `DefaultMeterIdPrefixFunction` which `GrpcMeterIdPrefixFunction` and `RetrofitMeterIdPrefixFunction` extend.

Result:
- Close line#2762
- You can now use `GrpcMeterIdPrefixFunction` to add `grpc.status` tag to the metric easily.

To-do:
- The marking of successes and failures is not working well when
  `MetricCollectingClient` is between `RetryingClient` and `HttpClientDelegate`.
minwoox added a commit to minwoox/armeria that referenced this issue Aug 6, 2020
@minwoox
Add GrpcMeterIdPrefixFunction
fdebdf8 
Motivation:
It'd be nice to have a dedicated `MeterIdPrefixFunction` implementation for gRPC,
so that the tag for grpc-status is added automatically.

Modifications:
- Add `GrpcMeterIdPrefixFunction`.
- Insert `GrpcWebTrailersExtrator` before `HttpClientDelegate` when the gRPC-web client is created.
- Remove `GrpcWebUtil.parseTrailers()` which was used to extract web trailers from RetringClient.
  - Could use `GrpcWebUtil.trailers(ctx)` to get gRPC web trailers instead.
- Add `DefaultMeterIdPrefixFunction` which `GrpcMeterIdPrefixFunction` and `RetrofitMeterIdPrefixFunction` extend.
- Add `AbstractClientOptionsBuilder.clearDecorator()` to remove decorators set.
- Add meter tags in the Alphabet order.

Result:
- Close line#2762
- You can now use `GrpcMeterIdPrefixFunction` to add `grpc.status` tag to the metric easily.

To-do:
- The marking of successes and failures is not working well when
  `MetricCollectingClient` is between `RetryingClient` and `HttpClientDelegate`.
minwoox added a commit to minwoox/armeria that referenced this issue Aug 6, 2020
@minwoox
Add GrpcMeterIdPrefixFunction
02c6f09 
Motivation:
It'd be nice to have a dedicated `MeterIdPrefixFunction` implementation for gRPC,
so that the tag for grpc-status is added automatically.

Modifications:
- Add `GrpcMeterIdPrefixFunction`.
- Insert `GrpcWebTrailersExtrator` before `HttpClientDelegate` when the gRPC-web client is created.
- Remove `GrpcWebUtil.parseTrailers()` which was used to extract web trailers from RetringClient.
  - Could use `GrpcWebUtil.trailers(ctx)` to get gRPC web trailers instead.
- Add `DefaultMeterIdPrefixFunction` which `GrpcMeterIdPrefixFunction` and `RetrofitMeterIdPrefixFunction` extend.
- Add `AbstractClientOptionsBuilder.clearDecorator()` to remove decorators set.
- Add meter tags in the Alphabet order.
- Add grpc-status header to the unframed grpc service response headers.

Result:
- Close line#2762
- You can now use `GrpcMeterIdPrefixFunction` to add `grpc.status` tag to the metric easily.

To-do:
- The marking of successes and failures is not working well when
  `MetricCollectingClient` is between `RetryingClient` and `HttpClientDelegate`.
minwoox added a commit to minwoox/armeria that referenced this issue Aug 6, 2020
@minwoox
Add GrpcMeterIdPrefixFunction
4f9b4ee 
Motivation:
It'd be nice to have a dedicated `MeterIdPrefixFunction` implementation for gRPC,
so that the tag for grpc-status is added automatically.

Modifications:
- Add `GrpcMeterIdPrefixFunction`.
- Insert `GrpcWebTrailersExtrator` before `HttpClientDelegate` when the gRPC-web client is created.
- Remove `GrpcWebUtil.parseTrailers()` which was used to extract web trailers from RetringClient.
  - Could use `GrpcWebUtil.trailers(ctx)` to get gRPC web trailers instead.
- Add `DefaultMeterIdPrefixFunction` which `GrpcMeterIdPrefixFunction` and `RetrofitMeterIdPrefixFunction` extend.
- Add `AbstractClientOptionsBuilder.clearDecorator()` to remove decorators set.
- Add meter tags in the Alphabet order.
- Add grpc-status header to the unframed grpc service response headers.

Result:
- Close line#2762
- You can now use `GrpcMeterIdPrefixFunction` to add `grpc.status` tag to the metric easily.

To-do:
- The marking of successes and failures is not working well when
  `MetricCollectingClient` is between `RetryingClient` and `HttpClientDelegate`.
@minwoox minwoox mentioned this issue Aug 6, 2020
Merged
trustin pushed a commit that referenced this issue Aug 13, 2020
@minwoox
Add GrpcMeterIdPrefixFunction (#2971)
1929ba0 
Motivation:
It'd be nice to have a dedicated `MeterIdPrefixFunction` implementation for gRPC,
so that the tag for grpc-status is added automatically.

Modifications:
- Add `GrpcMeterIdPrefixFunction`.
- Insert `GrpcWebTrailersExtrator` before `HttpClientDelegate` when the gRPC-web client is created.
- Remove `GrpcWebUtil.parseTrailers()` which was used to extract web trailers from RetringClient.
  - Could use `GrpcWebTrailers.get(ctx)` to get gRPC web trailers instead.
- Add `DefaultMeterIdPrefixFunction` which `GrpcMeterIdPrefixFunction` and `RetrofitMeterIdPrefixFunction` extend.
- Add `AbstractClientOptionsBuilder.clearDecorator()` to remove decorators set.
- Add meter tags in the Alphabet order.
- Add grpc-status header to the unframed grpc service response headers.

Result:
- Close #2762
- You can now use `GrpcMeterIdPrefixFunction` to add `grpc.status` tag to the metric easily.

To-do:
- The marking of successes and failures is not working well when
  `MetricCollectingClient` is between `RetryingClient` and `HttpClientDelegate`.
@okue
Copy link
Member

okue commented Aug 27, 2020

When using gRPC with armeria-spring-boot, all services are decorated with MeterIdPrefixFunction.ofDefault.
I'd be happy if GrpcMeterIdPrefixFunction is used for gRPC services.

https://github.com/line/armeria/blob/armeria-1.0.0/spring/boot2-autoconfigure/src/main/java/com/linecorp/armeria/internal/spring/ArmeriaConfigurationUtil.java#L131-L132

@okue okue mentioned this issue Aug 31, 2020
Merged
trustin pushed a commit that referenced this issue Sep 9, 2020
@okue
Use MeterIdPrefixFunction bean when integrated with Spring Boot (#3034)
e5c4e81 
Motivation:
Armeria Spring Integration always uses `MeterIdPrefixFunction.ofDefault` when `enable-metrics` is set to true.
It'd be good if users can configure the `MeterIdPrefixFunction` Armeria uses.

and #2762 (comment)

Modifications:
- Add an argument `Optional<MeterIdPrefixFunction>` to `ArmeriaAutoConfiguration.armeriaServer`
- Add an argument `MeterIdPrefixFunction` to `ArmeriaConfigurationUtil.configureServerWithArmeriaSettings`

Result:
The `MeterIdPrefixFunction` bean which users register will be used.
If the bean is not present, `MeterIdPrefixFunction.ofDefault` will be used.
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this issue Sep 19, 2020
@okue
Add MeterIdPrefixFunction.andThen to customize MeterIdPrefix using Re…
77cb0cd 
…questLog (line#2839)

Preparation for line#2762.

Motivations:
Currently, `MeterIdPrefixFunction` can be customized with [`andThen`](https://github.com/line/armeria/blob/armeria-0.99.7/core/src/main/java/com/linecorp/armeria/common/metric/MeterIdPrefixFunction.java#L179-L193) which takes a function of type `(MeterRegistry, MeterIdPrefix) -> MeterIdPrefix`.
But, without taking `RequestLog`, we cannot append header values to `MeterIdPrefix`.

Modifications:
- Add `andThen: (MeterRegistry, RequestOnlyLog, MeterIdPrefix) -> MeterIdPrefix`
- Deprecate the original version `andThen(BiFunction<MeterRegistry, MeterIdPrefix, MeterIdPrefix>)`

Results:
Users can customize `MeterIdPrefixFunction` easily like

```java
MeterIdPrefixFunction
        .ofDefault("hoge")
        .andThen((registry, log, id) -> {
            if (log instanceof RequestLog) {
                return id.withTags("grpc-status", log.responseHeaders().get("grpc-status"));
            } else {
                return id;
            }
        });
```
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this issue Sep 19, 2020
@minwoox
Add GrpcMeterIdPrefixFunction (line#2971)
15d52d8 
Motivation:
It'd be nice to have a dedicated `MeterIdPrefixFunction` implementation for gRPC,
so that the tag for grpc-status is added automatically.

Modifications:
- Add `GrpcMeterIdPrefixFunction`.
- Insert `GrpcWebTrailersExtrator` before `HttpClientDelegate` when the gRPC-web client is created.
- Remove `GrpcWebUtil.parseTrailers()` which was used to extract web trailers from RetringClient.
  - Could use `GrpcWebTrailers.get(ctx)` to get gRPC web trailers instead.
- Add `DefaultMeterIdPrefixFunction` which `GrpcMeterIdPrefixFunction` and `RetrofitMeterIdPrefixFunction` extend.
- Add `AbstractClientOptionsBuilder.clearDecorator()` to remove decorators set.
- Add meter tags in the Alphabet order.
- Add grpc-status header to the unframed grpc service response headers.

Result:
- Close line#2762
- You can now use `GrpcMeterIdPrefixFunction` to add `grpc.status` tag to the metric easily.

To-do:
- The marking of successes and failures is not working well when
  `MetricCollectingClient` is between `RetryingClient` and `HttpClientDelegate`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
new feature
Projects
None yet
Milestone
1.0.0
Development

Successfully merging a pull request may close this issue.

Add GrpcMeterIdPrefixFunction minwoox/armeria
4 participants
@trustin @ikhoon @okue @minwoox