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

Use labels in prometheus metrics #1681

Merged
merged 3 commits into from
May 5, 2022
Merged

Use labels in prometheus metrics #1681

merged 3 commits into from
May 5, 2022

Conversation

rmattis
Copy link
Contributor

@rmattis rmattis commented Jan 13, 2022

The current prometheus implementation doesn't allow the usage of Prometheus labels.

To overcome this, we used the MapperConfig feature to allow Dropwizard parsing the labels out of the metrics path.

To enable the feature it is necessary to set metrics.prometheus.labels.enabled to true. Otherwise it'll just stay as it is.

@Net-burst Net-burst requested review from And1sS and CTMBNara and removed request for And1sS January 25, 2022 08:10
@SerhiiNahornyi SerhiiNahornyi requested a review from And1sS March 29, 2022 14:13
And1sS
And1sS requested changes Apr 4, 2022
View reviewed changes
src/main/java/org/prebid/server/spring/config/PrometheusMapperConfiguration.java Outdated
Comment on lines 30 to 46
public SampleBuilder labelsSampleBuilder() {
final List<MapperConfig> mapperConfigs = new ArrayList<>();

mapperConfigs.addAll(accountModuleMapperConfigs());
mapperConfigs.addAll(moduleMapperConfigs());
mapperConfigs.addAll(accountMapperConfigs());
mapperConfigs.addAll(adapterMapperConfigs());
// other
mapperConfigs.add(requestsOk);
mapperConfigs.add(bidderCardinality);
mapperConfigs.addAll(privacyMappingConfigs());
mapperConfigs.addAll(vertxResponseConfig);
mapperConfigs.addAll(vertxRequestConfigs);
mapperConfigs.add(vertxServer);

return new CustomMappingSampleBuilder(mapperConfigs);
}
Copy link
Member

Choose a reason for hiding this comment

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

Lets utilize Spring and make this method simpler:

@Bean
    @ConditionalOnProperty(prefix = "metrics.prometheus.labels", name = "enabled", havingValue = "true")
    public SampleBuilder labelsSampleBuilder(List<MapperConfig> mapperConfigs) {
        return new CustomMappingSampleBuilder(mapperConfigs);
    }

Copy link
Contributor Author

@rmattis rmattis Apr 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Done 👍🏻 But I had to use a List<List<MapperConfig>> because Spring didn't like the max of

@Bean
public MapperConfig mapperConfig() {}

and

@Bean
public List<MapperConfig> mapperConfig() {}

so that the last Bean doesn't overwrite all other Bean declarations

src/main/java/org/prebid/server/spring/config/PrometheusMapperConfiguration.java Outdated
Comment on lines 49 to 53
private final MapperConfig requestsOk = new MapperConfig(
"requests.*.*",
"requests.status.type",
Map.of("status", "${0}", "type", "${1}")
);
Copy link
Member

Choose a reason for hiding this comment

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

Let's make all these fields Spring beans and extract groups of related mappings to separate configuration.
Also, we are not breaking line for );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

src/main/java/org/prebid/server/spring/config/PrometheusMapperConfiguration.java Outdated
Comment on lines 63 to 71
private final List<MapperConfig> vertxRequestConfigs = Stream.of(
"get", "post", "head", "delete", "connect", "options", "patch", "put", "other", "trace"
).map(request -> new MapperConfig(
"vertx.http.clients." + request + "-requests",
"vertx.http.clients.requests_by_type",
Map.of("request", request)
)

).collect(Collectors.toList());
Copy link
Member

Choose a reason for hiding this comment

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

with our code style it would be:

Stream.of("get", "post", "head", "delete", "connect", "options", "patch", "put", "other", "trace")
    .map(request -> new MapperConfig(
                 "vertx.http.clients." + request + "-requests",
                  "vertx.http.clients.requests_by_type",
                  Map.of("request", request)))
    .collect(Collectors.toList());

Also, this should be a bean provider method, as i said before:

@Bean
   public List<MapperConfig> vertxRequestMapperConfigs() {
       return Stream.of("get", "post", "head", "delete", "connect", "options", "patch", "put", "other", "trace")
               .map(request -> new MapperConfig(
                       "vertx.http.clients." + request + "-requests",
                       "vertx.http.clients.requests_by_type",
                       Map.of("request", request)))
               .collect(Collectors.toList());
   }

Please, refactor other mappings to follow this pattern.

src/main/java/org/prebid/server/spring/config/PrometheusMapperConfiguration.java
Comment on lines +91 to +109
final MapperConfig privacyTcfMissing = new MapperConfig(
"privacy.tcf.missing",
"privacy.tcf.errors",
Map.of("error", "missing"));

final MapperConfig privacyTcfInvalid = new MapperConfig(
"privacy.tcf.invalid",
"privacy.tcf.errors",
Map.of("error", "invalid"));

final MapperConfig privacyVendorList = new MapperConfig(
"privacy.tcf.*.vendorlist.*",
"privacy.tcf.vendorlist",
Map.of("tcf", "${0}", "status", "${1}"));

final MapperConfig privacyTcfStatus = new MapperConfig(
"privacy.tcf.*.*",
"privacy.tcf.${1}",
Map.of("tcf", "${0}"));
Copy link
Member

Choose a reason for hiding this comment

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

All of these should be separate beans. Could be extracted to separate configuration.

src/main/java/org/prebid/server/spring/config/PrometheusMapperConfiguration.java Outdated
Comment on lines 142 to 195
final MapperConfig accountRequestTime = new MapperConfig(
"account.*.adapter.*.request_time",
"account.request_time",
Map.of("account", "${0}", "adapter", "${1}")
);

// example: account.<account>.adapter.appnexus.requests.gotbids
final MapperConfig accountAdapterRequests = new MapperConfig(
"account.*.adapter.*.requests.*",
"account.requests.responses",
Map.of("account", "${0}", "adapter", "${1}", "response", "${2}")
);

// example: account.<account>.adapter.appnexus.bids_received
final MapperConfig accountAdapterBidsReceived = new MapperConfig(
"account.*.adapter.*.bids_received",
"account.bids_received",
Map.of("account", "${0}", "adapter", "${1}")
);

// example: account.<account>.requests.type.openrtb2-web
final MapperConfig accountRequestTypeConfig = new MapperConfig(
"account.*.requests.type.*",
"account.requests.type",
Map.of("account", "${0}", "type", "${1}")
);

// example: account_<account>_adapter_pubmatic_prices
final MapperConfig accountPricesConfig = new MapperConfig(
"account.*.adapter.*.prices",
"account.prices",
Map.of("account", "${0}", "adapter", "${1}")
);

// example: adapter.yieldlab.response.validation.size.warn
final MapperConfig accountValidationConfig = new MapperConfig(
"account.*.response.validation.*.*",
"account.response.validation",
Map.of("account", "${0}", "validation", "${1}", "level", "${2}")
);

// example: account.<account>.requests
final MapperConfig accountCacheConfig = new MapperConfig(
"account.*.prebid_cache.requests.*",
"account.requests.type",
Map.of("account", "${0}", "result", "${1}")
);

// example: account.<account>.requests
final MapperConfig accountRequestsConfig = new MapperConfig(
"account.*.requests",
"account.requests.type",
Map.of("account", "${0}")
);
Copy link
Member

Choose a reason for hiding this comment

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

Same here

src/main/java/org/prebid/server/spring/config/PrometheusMapperConfiguration.java Outdated
Comment on lines 204 to 290
final MapperConfig adapterRequestTime = new MapperConfig(
"adapter.*.request_time",
"adapter.request_time",
Map.of("adapter", "${0}")
);

// example: adapter.ix.requests.type.openrtb2-web
final MapperConfig adapterRequestsType = new MapperConfig(
"adapter.*.requests.type.*",
"adapter.requests.type",
Map.of("adapter", "${0}", "type", "${1}")
);

// example: adapter.yieldlab.requests.nobid
// example: adapter.yieldlab.requests.timeout
// example: adapter.yieldlab.requests.unknown_error
// example: adapter.yieldlab.requests.gotbids
final MapperConfig adapterRequestsResult = new MapperConfig(
"adapter.*.requests.*",
"adapter.requests.result",
Map.of("adapter", "${0}", "result", "${1}")
);

// example: adapter.pubmatic.bids_received
final MapperConfig adapterBidsReceived = new MapperConfig(
"adapter.*.bids_received",
"adapter.bids_received",
Map.of("adapter", "${0}")
);

// example: adapter.pubmatic.price
final MapperConfig adapterPrices = new MapperConfig(
"adapter.*.prices",
"adapter.prices",
Map.of("adapter", "${0}")
);

// example: adapter.yieldlab.response.validation.size.warn
final MapperConfig adapterValidation = new MapperConfig(
"adapter.*.response.validation.*.*",
"adapter.response.validation",
Map.of("adapter", "${0}", "validation", "${1}", "level", "${2}")
);

// example: adapter.yieldlab.no_cookie_requests
final MapperConfig adapterNoCookieConfig = new MapperConfig(
"adapter.*.no_cookie_requests",
"adapter.no_cookie_requests",
Map.of("adapter", "${0}")
);

// example: adapter.ix.openrtb2-web.tcf.request_blocked
final MapperConfig adapterRequestTypeTcfConfig = new MapperConfig(
"adapter.*.*.tcf.*",
"adapter.tcf",
Map.of("adapter", "${0}", "type", "${1}", "tcf", "${2}")
);

// example: adapter.appnexus.banner.adm_bids_received
// example: adapter.appnexus.xNative.adm_bids_received
final MapperConfig adapterMediaType = new MapperConfig(
"adapter.*.*.*",
"adapter.media_type",
Map.of("adapter", "${0}", "mediaType", "${1}", "metric", "${2}")
);

// example: cookie_sync.rubicon.tcf.blocked
final MapperConfig adapterCookieSyncTcfConfig = new MapperConfig(
"cookie_sync.*.tcf.*",
"adapter.cookie_sync.tcf",
Map.of("adapter", "${0}", "tcf", "${1}")
);

// example: cookie_sync.appnexus.gen
// example: cookie_sync.ix.matches
final MapperConfig adapterGenCookieConfig = new MapperConfig(
"cookie_sync.*.*",
"adapter.cookie_sync.action",
Map.of("adapter", "${0}", "action", "${1}")
);

// example: usersync.triplelift.sets
final MapperConfig adapterUserSyncSets = new MapperConfig(
"usersync.*.*",
"adapter.usersync.action",
Map.of("adapter", "${0}", "action", "${1}")
);
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@muuki88
Copy link
Contributor

muuki88 commented Apr 11, 2022

I saw there are other changes in #1802

Would it be possible to merge this first when the comments are resolved?

@And1sS
Copy link
Member

And1sS commented Apr 11, 2022

I saw there are other changes in #1802

Would it be possible to merge this first when the comments are resolved?

Yeah, I will resolve merge conflicts in #1802

@SerhiiNahornyi
Copy link
Collaborator

@rmattis Pls merge master to this PR, to fix functional tests github action

@rmattis
Copy link
Contributor Author

rmattis commented May 5, 2022

@SerhiiNahornyi Done

@SerhiiNahornyi SerhiiNahornyi merged commit 197fa0d into prebid:master May 5, 2022
@rmattis rmattis deleted the prometheus-metric-labels branch May 12, 2022 11:45
@rmattis rmattis mentioned this pull request May 19, 2022
Merged
@muuki88 muuki88 mentioned this pull request Aug 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SerhiiNahornyi SerhiiNahornyi SerhiiNahornyi approved these changes

@And1sS And1sS And1sS approved these changes

@CTMBNara CTMBNara Awaiting requested review from CTMBNara

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rmattis @muuki88 @And1sS @SerhiiNahornyi