Add telemetry for Stapler dispatches #3688
Conversation
daniel-beck
added
the
needs-more-reviews
| private Object buildComponentInformation() { | ||
| Map<String, String> components = new TreeMap<>(); | ||
| VersionNumber core = Jenkins.getVersion(); | ||
| components.put("jenkins-core", core == null ? "" : core.toString()); |
There was a problem hiding this comment.
How would it be possible for Jenkins.getVersion to respond with null? 😕
There was a problem hiding this comment.
No idea, but it's @CheckForNull so I do that. The Javadoc of https://javadoc.jenkins.io/jenkins/model/Jenkins.html#getVersion-- mentions an example that does not apply, when you run Jenkins that way it's 2.146-SNAPSHOT or similar. Might be a very rare occurrence such as running unit test in an IDE, but I choose to be careful here just in case.
| } | ||
|
|
||
| private Object buildDispatches() { | ||
| Set<String> currentTraces = traces; |
There was a problem hiding this comment.
I thought with class variables this.traces had to be used rather than just the member name. Guess that's just what Java does nowadays 🤷♂️
There was a problem hiding this comment.
Qualified access is optional if it's unambiguous. That's why getters return foo but setters set this.foo = foo as the field is hidden by the parameter of the same name (foo = newFoo would also work here).
Pretty sure it's always been that way, but the oldest JLS I could find is version 3 for Java 6 (2006):
https://docs.oracle.com/javase/specs/jls/se6/html/expressions.html#15.11
It is also possible to refer to a field of the current instance or current class by using a simple name
https://docs.oracle.com/javase/specs/jls/se6/html/names.html#129350
If an expression name consists of a single Identifier, then there must be exactly one visible declaration denoting either a local variable, parameter or field in scope at the point at which the the Identifier occurs.
https://docs.oracle.com/javase/specs/jls/se6/html/names.html#34133
Some declarations may be shadowed in part of their scope by another declaration of the same name, in which case a simple name cannot be used to refer to the declared entity. … A declaration d of a field, local variable, method parameter, constructor parameter or exception handler parameter named n shadows the declarations of any other fields, local variables, method parameters, constructor parameters or exception handler parameters named n that are in scope at the point where d occurs throughout the scope of d.
|
|
||
| @Override | ||
| protected void record(StaplerRequest staplerRequest, String s) { | ||
| if (UsageStatistics.DISABLED || !Jenkins.get().isUsageStatisticsCollected()) { |
There was a problem hiding this comment.
I'm assuming this is just an optimization, since Telemetry is not reported regardless when these settings are present, correct?
There was a problem hiding this comment.
Yes. This covers two potential issues:
- Accumulation of trace information without ever being cleared out on submission, as there is no submission.
- Having disabled usage statistics, then enabling it. User expectation is likely that the next submission will only contain data collected from that point forward, not any older.
There was a problem hiding this comment.
I do something similar in https://github.com/jenkinsci/jenkins/pull/3687/files#diff-f2ee06c0611e993efba3c8330e1b98feR114 but that PR adds new core API to do the check so I didn't do it here to prevent merge conflicts. Should probably be cleaned up later.
There was a problem hiding this comment.
Please add a TODO comment here reminding people to use that API once merged.
| // do not collect traces while usage statistics are disabled | ||
| return; | ||
| } | ||
| traces.add(s); |
There was a problem hiding this comment.
Just sanity checking here, is there any reason this code doesn't flush traces after Telemetry has been reported? I don't think it should matter since it's a Set<> anyways, but just thinking out loud a bit here 😸
There was a problem hiding this comment.
Essentially done by replacing the set in https://github.com/jenkinsci/jenkins/pull/3688/files#diff-db538ffb33f05cbc1d77afb1fe990dccR88 and letting gc do the rest.
There was a problem hiding this comment.
As a side benefit, flushing will result in us knowing how frequently certain routes are accessed, to assess their importance (e.g. when looking into performance).
| @@ -0,0 +1,7 @@ | |||
| <?jelly escape-by-default='true'?> | |||
| <j:jelly xmlns:j="jelly:core"> | |||
| Stapler is the web framework used by Jenkins. | |||
|
|
||
| for (PluginWrapper plugin : Jenkins.get().pluginManager.getPlugins()) { | ||
| if (plugin.isActive()) { | ||
| components.put(plugin.getShortName(), plugin.getVersion()); |
There was a problem hiding this comment.
This seems like boilerplate. Surely it should just be pulled up into Telemetry?
There was a problem hiding this comment.
Depends on what we are collecting whether we need plugins. Certainly neither security-system-properties nor the neighboring PR with the Accept-Language headers would need this information.
Since there's a (limited) potential to collect information private to an organization (see some parts of the usage stats, showing versions like 1.5-orgname-3), I'd prefer we define the full set of required information in each collector for now.
|
|
||
| @Override | ||
| protected void record(StaplerRequest staplerRequest, String s) { | ||
| if (UsageStatistics.DISABLED || !Jenkins.get().isUsageStatisticsCollected()) { |
There was a problem hiding this comment.
Please add a TODO comment here reminding people to use that API once merged.
|
|
||
| private Object buildDispatches() { | ||
| Set<String> currentTraces = traces; | ||
| traces = new TreeSet<>(); |
There was a problem hiding this comment.
This is not really thread-safe; you can wind up missing some traces this way, if anyone cares.
Simpler and safer to drop volatile and just make this method synchronized.
There was a problem hiding this comment.
Does not likely matter much, just FYI. Generally avoid volatile.
| } | ||
| } | ||
|
|
||
| private static volatile Set<String> traces = new TreeSet<>(); |
There was a problem hiding this comment.
Should be an instance variable, not static, and accessed from StaplerTrace via ExtensionList.lookupSingleton.
There was a problem hiding this comment.
Lack of synchronization led to #3700 FTR.
* Add telemetry for Stapler dispatches * Add TODO * Released version of Stapler (cherry picked from commit d1cbb20)
I'm looking into modernizing Stapler. This is related to Stephen's work from last year that aimed to make Stapler more declarative. First however I'd like to gather some information about how Stapler is used "in the wild" to determine the impact in terms of changes required to fully adapt to any potential changes I'm looking into. For that reason, this trial collect anonymized dispatch information similar to
X-Stapler-Trace, but without the actual string values etc. (i.e. this does not collect user data)Downstream of jenkinsci/stapler#148
Sample payload submitted (formatted for readability):
Proposed changelog entries
Submitter checklist
* Use the
Internal:prefix if the change has no user-visible impact (API, test frameworks, etc.)