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

PAYARA-3830 Microprofile Metrics 2.0 #4076

Merged
merged 14 commits into from Jul 19, 2019

Conversation

@Cousjava
Copy link
Member

commented Jul 9, 2019

@Cousjava Cousjava added this to the 5.193 milestone Jul 9, 2019

@Cousjava

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

Jenkins test please

@Cousjava Cousjava force-pushed the Cousjava:PAYARA-3830-metrics-20 branch from 0c0e610 to 70c028d Jul 10, 2019

@Cousjava

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2019

Jenkins test please

@Cousjava

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

Jenkins test please

@Cousjava

This comment has been minimized.

Copy link
Member Author

commented Jul 15, 2019

requires payara/Payara_PatchedProjects#257 to be merged first

@Cousjava

This comment has been minimized.

Copy link
Member Author

commented Jul 15, 2019

Jenkins test please

@Cousjava

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

Jenkins test please

if (splitIndex == -1) {
throw new IllegalArgumentException("invalid tag: " + stringtags[i] + ", tags must be in the form key=value");
} else {
tags[i] = new Tag(stringtags[i].substring(0, splitIndex), stringtags[i].substring(splitIndex + 1));

This comment has been minimized.

Copy link
@pdudits

pdudits Jul 16, 2019

Contributor

I always love small independent methods, that can be unit tested, especially when they do any index access.

For example, what it returns for tagsFromString(new String[] {"key=value", "surprise="}) ? ;)

@jGauravGupta jGauravGupta self-requested a review Jul 17, 2019

PAYARA-3830 Futher requested changes
Changes were requested by Patrik and Gaurav

@Cousjava Cousjava force-pushed the Cousjava:PAYARA-3830-metrics-20 branch from b512488 to 88ddcdb Jul 17, 2019

@Cousjava

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2019

Jenkins test please

@Cousjava Cousjava requested a review from jGauravGupta Jul 17, 2019

@Cousjava Cousjava requested a review from pdudits Jul 19, 2019

import org.eclipse.microprofile.metrics.Timer;
import org.eclipse.microprofile.metrics.annotation.Metric;

@ApplicationScoped
public class MetricsHelper {

@ConfigProperty(name="mp.metrics.tags")
String globalTags;

This comment has been minimized.

Copy link
@pdudits

pdudits Jul 19, 2019

Contributor

It's not required to define this property, is it?

In such case, I believe, you must inject Optional<String> to prevent injection failures.

@@ -67,34 +69,95 @@

@Produces
private Counter counter(InjectionPoint ip) {
return registry.counter(helper.metadataOf(ip, Counter.class));
Metric annotation = ip.getAnnotated().getAnnotation(Metric.class);
if (annotation != null) {

This comment has been minimized.

Copy link
@pdudits

pdudits Jul 19, 2019

Contributor

I see some method to be extracted here. This block could be extracted and reused in downward methods, but it's type definition would be horrendous (T register(BiFunction<Metadata,Tag[],T> factory1,BiFunction<String,Tag[]> factory2, .Function<String,T> factory3)... actually I was expecting to type much more).
You could at least extract the condition, and the construction of default name into their own methods

@Cousjava Cousjava force-pushed the Cousjava:PAYARA-3830-metrics-20 branch from fbf02b5 to 91d7d20 Jul 19, 2019

@Cousjava

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2019

Jenkins test please

@Cousjava Cousjava merged commit df8ebd0 into payara:master Jul 19, 2019

59 checks passed

Payara Quick Build and Test Quick build and test passed!
Details
security/snyk - api/payara-api/pom.xml (payara-ci) No new issues
Details
security/snyk - api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admingui/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ant-tasks/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/appclient/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/batch/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/common/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/concurrent/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/connectors/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/core/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ejb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/extras/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/featuresets/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ha/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/installer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/javaee-api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jdbc/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/load-balancer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/orb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/payara-appserver-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/persistence/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/registration/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/security/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/transaction/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/web/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/webservices/pom.xml (payara-ci) No new issues
Details
security/snyk - copyright/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/cluster/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/common/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/core/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/diagnostics/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/hk2/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/payara-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources-l10n/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/security/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/test-utils/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - pom.xml (payara-ci) No new issues
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.