From 34dc4375dadd04fc28998462d6bad805f5f00adb Mon Sep 17 00:00:00 2001 From: rob Date: Wed, 22 Feb 2023 15:22:58 +1300 Subject: [PATCH] Tidy existing given that lineNumber in SpiProfileLocationFactory always comes in as 0 The history behind this is that originally there was a "profileId" concept where that was a int value that could be used as an "id" for a transactional method that we wanted to profile. That went away and then the idea was that bytecode enhancement would determine the line number and we would use that but that was a bad idea and we ended up with lineNumber always as 0. These days with StackWalker and DProfileLocation we can do this better that way so yeah. A followup to this is to add to the api a boolean flag as to whether a profile location should be with line numbers (which can change frequently due to refactoring etc) --- .../service/SpiProfileLocationFactory.java | 16 ++++++++++++-- .../server/profile/DProfileLocation.java | 22 +------------------ .../profile/DProfileLocationFactory.java | 9 +++----- .../server/profile/DTimedProfileLocation.java | 3 +-- .../server/profile/UtilLocation.java | 11 +++++++++- .../profile/BasicProfileLocationTest.java | 6 ++--- .../server/profile/UtilLocationTest.java | 12 +++++++--- 7 files changed, 41 insertions(+), 38 deletions(-) diff --git a/ebean-api/src/main/java/io/ebean/service/SpiProfileLocationFactory.java b/ebean-api/src/main/java/io/ebean/service/SpiProfileLocationFactory.java index e5626a69e..a2f269641 100644 --- a/ebean-api/src/main/java/io/ebean/service/SpiProfileLocationFactory.java +++ b/ebean-api/src/main/java/io/ebean/service/SpiProfileLocationFactory.java @@ -13,9 +13,21 @@ public interface SpiProfileLocationFactory { ProfileLocation create(); /** - * Create a profile location with a line number. + * Create with a given label - used only with {@code @Transaction}. + * + * @param label the label for the transaction */ - ProfileLocation create(int lineNumber, String label); + ProfileLocation create(String label); + + /** + * Create a profile location with a line number. + * + * @param lineNumber always 0 + * @param label the label for the transaction + */ + default ProfileLocation create(int lineNumber, String label) { + return create(label); + } /** * Create a known location. diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/profile/DProfileLocation.java b/ebean-core/src/main/java/io/ebeaninternal/server/profile/DProfileLocation.java index ed56cf89f..0f51e078a 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/profile/DProfileLocation.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/profile/DProfileLocation.java @@ -15,18 +15,9 @@ class DProfileLocation implements ProfileLocation { private String fullLocation; private String location; private String label; - private final int lineNumber; private int traceCount; DProfileLocation() { - this(0); - } - - /** - * Create with a given line number. - */ - DProfileLocation(int lineNumber) { - this.lineNumber = lineNumber; } @Override @@ -96,19 +87,8 @@ class DProfileLocation implements ProfileLocation { private String filter(Stream frames) { return frames.filter(StackWalkFilter.filter()) .findFirst() - .map(line -> withLineNumber(line.toString())) + .map(Object::toString) .orElse(UNKNOWN); } - private String withLineNumber(String traceLine) { - if (lineNumber == 0) { - return traceLine; - } else if (traceLine.endsWith(":1)")) { - return traceLine.substring(0, traceLine.length() - 3) + ":" + lineNumber + ")"; - } else if (traceLine.contains(":")) { - return traceLine; - } else { - return traceLine.substring(0, traceLine.length() - 1) + ":" + lineNumber + ")"; - } - } } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/profile/DProfileLocationFactory.java b/ebean-core/src/main/java/io/ebeaninternal/server/profile/DProfileLocationFactory.java index cc6e19bd3..915a96163 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/profile/DProfileLocationFactory.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/profile/DProfileLocationFactory.java @@ -2,7 +2,6 @@ package io.ebeaninternal.server.profile; import io.ebean.ProfileLocation; import io.ebean.metric.MetricFactory; -import io.ebean.metric.TimedMetric; import io.ebean.service.SpiProfileLocationFactory; /** @@ -16,11 +15,9 @@ public final class DProfileLocationFactory implements SpiProfileLocationFactory } @Override - public ProfileLocation create(int lineNumber, String label) { - - TimedMetric timedMetric = MetricFactory.get().createTimedMetric("txn.named." + label); - - DTimedProfileLocation loc = new DTimedProfileLocation(lineNumber, label, timedMetric); + public ProfileLocation create(String label) { + final var timedMetric = MetricFactory.get().createTimedMetric("txn.named." + label); + final var loc = new DTimedProfileLocation(label, timedMetric); TimedProfileLocationRegistry.register(loc); return loc; } diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/profile/DTimedProfileLocation.java b/ebean-core/src/main/java/io/ebeaninternal/server/profile/DTimedProfileLocation.java index 9039a6751..cbbf3ad8d 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/profile/DTimedProfileLocation.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/profile/DTimedProfileLocation.java @@ -15,8 +15,7 @@ final class DTimedProfileLocation extends DProfileLocation implements TimedProfi private String fullName; private String reportName; - DTimedProfileLocation(int lineNumber, String label, TimedMetric timedMetric) { - super(lineNumber); + DTimedProfileLocation(String label, TimedMetric timedMetric) { this.label = label; this.timedMetric = timedMetric; this.overrideMetricName = "".equals(label); diff --git a/ebean-core/src/main/java/io/ebeaninternal/server/profile/UtilLocation.java b/ebean-core/src/main/java/io/ebeaninternal/server/profile/UtilLocation.java index 65ab1ce4d..a5df881b3 100644 --- a/ebean-core/src/main/java/io/ebeaninternal/server/profile/UtilLocation.java +++ b/ebean-core/src/main/java/io/ebeaninternal/server/profile/UtilLocation.java @@ -3,14 +3,23 @@ package io.ebeaninternal.server.profile; final class UtilLocation { static String loc(String full) { + return loc(full, false); + } + + static String loc(String full, boolean includeLineNumber) { final int pos = full.lastIndexOf('('); if (pos > -1) { - return full.substring(0, pos); + return !includeLineNumber ? full.substring(0, pos) : full.substring(0, pos) + lineNumberFrom(full); } else { return full; } } + private static String lineNumberFrom(String full) { + int pos = full.lastIndexOf(':'); + return pos == -1 ? "" : full.substring(pos, full.length() - 1); + } + static String label(String location) { return trimInit(shortDesc(location)); } diff --git a/ebean-core/src/test/java/io/ebeaninternal/server/profile/BasicProfileLocationTest.java b/ebean-core/src/test/java/io/ebeaninternal/server/profile/BasicProfileLocationTest.java index 7e89b08a8..c7ab83034 100644 --- a/ebean-core/src/test/java/io/ebeaninternal/server/profile/BasicProfileLocationTest.java +++ b/ebean-core/src/test/java/io/ebeaninternal/server/profile/BasicProfileLocationTest.java @@ -16,7 +16,7 @@ class BasicProfileLocationTest { @Test void metricNameFromOverride() { - DTimedProfileLocation loc = new DTimedProfileLocation(12, "", MetricFactory.get().createTimedMetric("a.b.c")); + DTimedProfileLocation loc = new DTimedProfileLocation("", MetricFactory.get().createTimedMetric("a.b.c")); loc.initWith("foo.label"); loc.add(42); @@ -39,7 +39,7 @@ class BasicProfileLocationTest { @Test void metricNameFromTimed() { - DTimedProfileLocation loc = new DTimedProfileLocation(12, "foo", MetricFactory.get().createTimedMetric("a.b.c")); + DTimedProfileLocation loc = new DTimedProfileLocation("foo", MetricFactory.get().createTimedMetric("a.b.c")); loc.add(42); BasicMetricVisitor visitor = new BasicMetricVisitor("v", naming); @@ -52,7 +52,7 @@ class BasicProfileLocationTest { @Test void obtain() { - DProfileLocation loc = new DTimedProfileLocation(12, "foo", MetricFactory.get().createTimedMetric("junk")); + DProfileLocation loc = new DTimedProfileLocation("foo", MetricFactory.get().createTimedMetric("junk")); assertThat(loc.obtain()).isTrue(); assertThat(loc.fullLocation()).endsWith("org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:725)"); diff --git a/ebean-core/src/test/java/io/ebeaninternal/server/profile/UtilLocationTest.java b/ebean-core/src/test/java/io/ebeaninternal/server/profile/UtilLocationTest.java index 778379c59..94eac9135 100644 --- a/ebean-core/src/test/java/io/ebeaninternal/server/profile/UtilLocationTest.java +++ b/ebean-core/src/test/java/io/ebeaninternal/server/profile/UtilLocationTest.java @@ -5,17 +5,23 @@ import org.junit.jupiter.api.Test; import static org.assertj.core.api.Assertions.assertThat; -public class UtilLocationTest { +class UtilLocationTest { @Test - public void label() { + void label() { Assertions.assertThat(UtilLocation.label("foo")).isEqualTo("foo"); assertThat(UtilLocation.label("ProfileLocationTest$Other.")).isEqualTo("ProfileLocationTest$Other.init"); } @Test - public void loc() { + void loc() { assertThat(UtilLocation.loc("org.foo.MyFoo.doIt(MyFoo.java:12)")).isEqualTo("org.foo.MyFoo.doIt"); assertThat(UtilLocation.label("org.foo.MyFoo.doIt")).isEqualTo("MyFoo.doIt"); } + + @Test + void locWithLineNumber() { + assertThat(UtilLocation.loc("org.foo.MyFoo.doIt(MyFoo.java:12)", true)).isEqualTo("org.foo.MyFoo.doIt:12"); + assertThat(UtilLocation.label("org.foo.MyFoo.doIt:12")).isEqualTo("MyFoo.doIt:12"); + } }