Skip to content

Conversation

@ikhoon
Copy link
Contributor

@ikhoon ikhoon commented Aug 25, 2022

Motivation:

To diagnose an exception error on the client-side, request logs are important to check request parameters, response duration and application errors.

Modifications:

  • Add RequestLogConfig which can limit the sampling rate, adjust target groups, and set the logging levels.
  • TransientServiceOption.WITH_SERVICE_LOGGING is specified to HealthCheckService or PrometheusExpositionService
    if METRICS or HEALTH group is defined in RequestLogConfig

Result:

  • Logging configuration example:
    {
      "targetGroups": [ "API", "HEALTH" ],
      "loggerName": "centraldogma.test",
      "requestLogLevel": "DEBUG",
      "successfulResponseLevel": "DEBUG",
      "failureResponseLevel": "ERROR",
      "successSamplingRate": 0.4,
      "failureSamplingRate": 0.6
    }
  • Fixes Support for LoggingService in Central Dogma server #718

Motivation:

TBU

Modifications:

TBU

Result:

Logging configuration example:
```json
{
  "targetGroups": [ "API", "HEALTH" ],
  "loggerName": "centraldogma.test",
  "requestLogLevel": "DEBUG",
  "successfulResponseLevel": "DEBUG",
  "failureResponseLevel": "ERROR",
  "successSamplingRate": 0.4,
  "failureSamplingRate": 0.6
}
```
@ikhoon ikhoon added this to the 0.57.1 milestone Aug 25, 2022
@codecov
Copy link

codecov bot commented Aug 25, 2022

Codecov Report

Attention: Patch coverage is 80.00000% with 21 lines in your changes missing coverage. Please review.

Project coverage is 70.40%. Comparing base (f1a1c03) to head (5a862e6).
Report is 297 commits behind head on main.

Files with missing lines Patch % Lines
...linecorp/centraldogma/server/RequestLogConfig.java 47.05% 18 Missing ⚠️
...com/linecorp/centraldogma/server/CentralDogma.java 94.82% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #719      +/-   ##
============================================
+ Coverage     70.31%   70.40%   +0.08%     
- Complexity     3418     3444      +26     
============================================
  Files           349      351       +2     
  Lines         13470    13565      +95     
  Branches       1454     1466      +12     
============================================
+ Hits           9472     9550      +78     
- Misses         3139     3153      +14     
- Partials        859      862       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

logHealthCheck = true;
logMetrics = true;
} else {
for (RequestLogGroup logGroup : requestLogGroups) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Is it possible that users may want to configure different logging parameters for different paths?

e.g.

- METRIC: debug
- API: info
- ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh... It is so nice! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can implement it by adding different LoggingServices.

Copy link
Contributor

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks! Left some suggestions. 😄


private final Set<RequestLogGroup> targetGroups;

private final LogLevel requestLogLevel;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we allow to set different settings per RequestLogGroup?

logHealthCheck = true;
logMetrics = true;
} else {
for (RequestLogGroup logGroup : requestLogGroups) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about also supporting the specified path? e.g. /api/v1

@ikhoon ikhoon modified the milestones: 0.58.1, 0.58.2 Nov 10, 2022
@ikhoon ikhoon modified the milestones: 0.58.2, 0.59.1 Dec 28, 2022
@jrhee17 jrhee17 modified the milestones: 0.59.1, 0.60.0, 0.60.1, 0.60.2 Feb 9, 2023
@trustin
Copy link
Member

trustin commented Mar 8, 2023

  • Could a user specify different logging settings for different target groups? For example, a user might want to lower the log level or rate for WEB but not for API.
  • Can we also add OTHERS, which is used for all other requests?
  • How about categories rather than target groups?

@jrhee17 jrhee17 modified the milestones: 0.61.0, 0.61.1 Apr 10, 2023
@minwoox minwoox removed this from the 0.61.1 milestone May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for LoggingService in Central Dogma server

4 participants