Add OSGi to usage statistics #4895
Conversation
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained
flow-server/src/main/java/com/vaadin/flow/server/osgi/OSGiAccess.java, line 283 at r1 (raw file):
} catch (Exception e) {
In case there is no such class there will be NoClassDefFoundError ( I think) which extends Throwable and doesn't extend Exception.
I'm not sure about this catch though: why do you catch everything ?
It seems that it's used only when inside OSGi env. So I don't see a reason to catch Exception.
flow-server/src/main/java/com/vaadin/flow/server/startup/ApplicationRouteRegistry.java, line 134 at r1 (raw file):
UsageStatistics.markAsUsed("flow/OSGi", OSGiAccess.getOSGiVersion());
I think that semantically this is not the proper place for this.
This functionality is quite specific for routes. And it's a bit weird to have usage statistic here.
May be the statistic may be moved to the OSGiAccess inside LazyOSGiDetector which detects OSGi env presence.
pleku
force-pushed
the
add_osgi_usage
branch
from
c145a37
to
c740fb8
Compare
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained
flow-server/src/main/java/com/vaadin/flow/server/osgi/OSGiAccess.java, line 283 at r1 (raw file):
Previously, denis-anisimov (Denis) wrote…
} catch (Exception e) {In case there is no such class there will be
NoClassDefFoundError( I think) which extendsThrowableand doesn't extendException.I'm not sure about this catch though: why do you catch everything ?
It seems that it's used only when inside OSGi env. So I don't see a reason to catchException.
I would rather have just broken usage statistics for OSGi than have broken usage statics for OSGi that also break OSGi usage.
flow-server/src/main/java/com/vaadin/flow/server/startup/ApplicationRouteRegistry.java, line 134 at r1 (raw file):
Previously, denis-anisimov (Denis) wrote…
UsageStatistics.markAsUsed("flow/OSGi", OSGiAccess.getOSGiVersion());I think that semantically this is not the proper place for this.
This functionality is quite specific for routes. And it's a bit weird to have usage statistic here.May be the statistic may be moved to the
OSGiAccessinsideLazyOSGiDetectorwhich detects OSGi env presence.
Done.
| @@ -263,10 +265,29 @@ private ServletContext createOSGiServletContext() { | |||
| private static boolean isInOSGi() { | |||
| try { | |||
| Class.forName("org.osgi.framework.FrameworkUtil"); | |||
|
|
|||
| UsageStatistics.markAsUsed("flow/osgi", getOSGiVersion()); | |||
|
|
|||
| return true; | |||
| } catch (ClassNotFoundException exception) { | |||
There was a problem hiding this comment.
| try { | ||
| Bundle osgiBundle = org.osgi.framework.FrameworkUtil.getBundle(Bundle.class); | ||
| return osgiBundle.getVersion().toString(); | ||
| } catch (Throwable throwable) { |
There was a problem hiding this comment.
pleku
force-pushed
the
add_osgi_usage
branch
from
c740fb8
to
91d5bcd
Compare
The OSGi detection seems impossible to fake from a test that doesn't have OSGi framework on the classpath, so no test for this.
pleku
force-pushed
the
add_osgi_usage
branch
from
91d5bcd
to
9598672
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3.
Reviewable status: 3 unresolved discussions, 1 of 1 LGTMs obtained
There was a problem hiding this comment.
Dismissed @vaadin-bot from 3 discussions.
Reviewable status:complete! all discussions resolved, 1 of 1 LGTMs obtained
|
SonarQube analysis reported 5 issues
Watch the comments in this conversation to review them. 2 extra issuesNote: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:
|
The OSGi detection seems impossible to fake from a test that doesn't have OSGi framework on the classpath, so no test for this.
|
Cherry-picking to |
The OSGi detection seems impossible to fake from a test that doesn't
have OSGi framework on the classpath, so no test for this.
This change is