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

Add telemetry for Stapler dispatches #3688

Merged
merged 4 commits into from Oct 12, 2018

Conversation

4 participants
@daniel-beck
Copy link
Member

daniel-beck commented Oct 9, 2018

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 stapler/stapler#148

Sample payload submitted (formatted for readability):

{
  "components": {
    "ace-editor": "1.1",
    "analysis-core": "1.95",
    "antisamy-markup-formatter": "1.5",
    "apache-httpcomponents-client-4-api": "4.5.5-3.0",
    "authentication-tokens": "1.3",
    "authorize-project": "1.3.0",
    "blueocean": "1.8.4",
    "blueocean-autofavorite": "1.2.2",
    "blueocean-bitbucket-pipeline": "1.8.4",
    "blueocean-commons": "1.8.4",
    "blueocean-config": "1.8.4",
    "blueocean-core-js": "1.8.4",
    "blueocean-dashboard": "1.8.4",
    "blueocean-display-url": "2.2.0",
    "blueocean-events": "1.8.4",
    "blueocean-git-pipeline": "1.8.4",
    "blueocean-github-pipeline": "1.8.4",
    "blueocean-i18n": "1.8.4",
    "blueocean-jira": "1.8.4",
    "blueocean-jwt": "1.8.4",
    "blueocean-personalization": "1.8.4",
    "blueocean-pipeline-api-impl": "1.8.4",
    "blueocean-pipeline-editor": "1.8.4",
    "blueocean-pipeline-scm-api": "1.8.4",
    "blueocean-rest": "1.8.4",
    "blueocean-rest-impl": "1.8.4",
    "blueocean-web": "1.8.4",
    "bouncycastle-api": "2.17",
    "branch-api": "2.0.20",
    "checkstyle": "3.50",
    "cloudbees-bitbucket-branch-source": "2.2.12",
    "cloudbees-folder": "6.6",
    "cobertura": "1.13",
    "code-coverage-api": "1.0.5",
    "command-launcher": "1.2",
    "credentials": "2.1.18",
    "credentials-binding": "1.16",
    "display-url-api": "2.2.0",
    "docker-commons": "1.13",
    "docker-workflow": "1.17",
    "durable-task": "1.26",
    "favorite": "2.3.2",
    "git": "3.9.1",
    "git-client": "2.7.3",
    "git-server": "1.7",
    "github": "1.29.2",
    "github-api": "1.92",
    "github-branch-source": "2.4.0",
    "handlebars": "1.1.1",
    "handy-uri-templates-2-api": "2.1.6-1.0",
    "htmlpublisher": "1.17",
    "jackson2-api": "2.8.11.3",
    "javadoc": "1.4",
    "jdk-tool": "1.1",
    "jenkins-core": "2.146-SNAPSHOT",
    "jenkins-design-language": "1.8.4",
    "jira": "3.0.2",
    "jquery": "1.12.4-0",
    "jquery-detached": "1.2.1",
    "jsch": "0.1.54.2",
    "junit": "1.26.1",
    "lockable-resources": "2.3",
    "mailer": "1.21",
    "matrix-auth": "2.3",
    "matrix-project": "1.13",
    "maven-plugin": "3.1.2",
    "mercurial": "2.4",
    "metrics": "4.0.2.2",
    "momentjs": "1.1.1",
    "nodelabelparameter": "1.7.2",
    "pipeline-build-step": "2.7",
    "pipeline-graph-analysis": "1.7",
    "pipeline-input-step": "2.8",
    "pipeline-milestone-step": "1.3.1",
    "pipeline-model-api": "1.3.2",
    "pipeline-model-declarative-agent": "1.1.1",
    "pipeline-model-definition": "1.3.2",
    "pipeline-model-extensions": "1.3.2",
    "pipeline-rest-api": "2.10",
    "pipeline-stage-step": "2.3",
    "pipeline-stage-tags-metadata": "1.3.2",
    "pipeline-stage-view": "2.10",
    "plain-credentials": "1.4",
    "pubsub-light": "1.12",
    "resource-disposer": "0.12",
    "scm-api": "2.2.8",
    "script-security": "1.46",
    "sse-gateway": "1.16",
    "ssh-agent": "1.17",
    "ssh-credentials": "1.14",
    "ssh-slaves": "1.26",
    "structs": "1.17",
    "throttle-concurrents": "2.0.1",
    "timestamper": "1.8.10",
    "token-macro": "2.5",
    "variant": "1.1",
    "workflow-aggregator": "2.6",
    "workflow-api": "2.29",
    "workflow-basic-steps": "2.11",
    "workflow-cps": "2.57",
    "workflow-cps-global-lib": "2.12",
    "workflow-durable-task-step": "2.22",
    "workflow-job": "2.25",
    "workflow-multibranch": "2.20",
    "workflow-scm-step": "2.7",
    "workflow-step-api": "2.16",
    "workflow-support": "2.20",
    "ws-cleanup": "0.34",
    "xunit": "2.3.0"
  },
  "dispatches": [
    "com.cloudbees.hudson.plugins.folder.Folder#doContextMenu",
    "com.cloudbees.hudson.plugins.folder.Folder: Dispatch",
    "com.cloudbees.hudson.plugins.folder.Folder: StaplerProxy.getTarget()",
    "com.cloudbees.jenkins.GitHubWebHook: Dispatch",
    "com.cloudbees.jenkins.GitHubWebHook: Index: doIndex",
    "com.cloudbees.jenkins.plugins.bitbucket.endpoints.BitbucketCloudEndpoint$DescriptorImpl#doFillCredentialsIdItems",
    "com.cloudbees.jenkins.plugins.bitbucket.endpoints.BitbucketCloudEndpoint$DescriptorImpl: Dispatch",
    "com.cloudbees.jenkins.plugins.sshagent.SSHAgentBuildWrapper$CredentialHolder$DescriptorImpl#doFillIdItems",
    "com.cloudbees.jenkins.plugins.sshagent.SSHAgentBuildWrapper$CredentialHolder$DescriptorImpl: Dispatch",
    "com.cloudbees.plugins.credentials.CredentialsProviderTypeRestriction$Includes$DescriptorImpl#doFillProviderItems",
    "com.cloudbees.plugins.credentials.CredentialsProviderTypeRestriction$Includes$DescriptorImpl#doFillTypeItems",
    "com.cloudbees.plugins.credentials.CredentialsProviderTypeRestriction$Includes$DescriptorImpl: Dispatch",
    "com.cloudbees.plugins.credentials.GlobalCredentialsConfiguration#doConfigure",
    "com.cloudbees.plugins.credentials.GlobalCredentialsConfiguration: Dispatch",
    "com.cloudbees.plugins.credentials.GlobalCredentialsConfiguration: Jelly index: com/cloudbees/plugins/credentials/GlobalCredentialsConfiguration/index.jelly",
    "com.cloudbees.plugins.credentials.PluginImpl#doDynamic(...)",
    "com.cloudbees.plugins.credentials.PluginImpl: Dispatch",
    "hudson.Plugin$DummyImpl#doDynamic(...)",
    "hudson.Plugin$DummyImpl: Dispatch",
    "hudson.logging.LogRecorder: Dispatch",
    "hudson.logging.LogRecorder: Jelly index: hudson/logging/LogRecorder/index.jelly",
    "hudson.logging.LogRecorderManager#getDynamic(...)",
    "hudson.logging.LogRecorderManager: Dispatch",
    "hudson.logging.LogRecorderManager: StaplerProxy.getTarget()",
    "hudson.model.AllView: Dispatch",
    "hudson.model.AllView: Jelly facet: hudson/model/View/ajaxBuildQueue.jelly",
    "hudson.model.AllView: Jelly facet: hudson/model/View/ajaxExecutors.jelly",
    "hudson.model.FreeStyleBuild: Dispatch",
    "hudson.model.FreeStyleBuild: Jelly index: hudson/model/AbstractBuild/index.jelly",
    "hudson.model.FreeStyleProject#doCheckNewName",
    "hudson.model.FreeStyleProject#doContextMenu",
    "hudson.model.FreeStyleProject#getDescriptorByName(String)",
    "hudson.model.FreeStyleProject#getDynamic(...)",
    "hudson.model.FreeStyleProject$DescriptorImpl#doCheckCustomWorkspace",
    "hudson.model.FreeStyleProject$DescriptorImpl: Dispatch",
    "hudson.model.FreeStyleProject: Dispatch",
    "hudson.model.FreeStyleProject: Jelly facet: hudson/model/AbstractItem/confirm-rename.jelly",
    "hudson.model.FreeStyleProject: Jelly facet: hudson/model/Job/configure.jelly",
    "hudson.model.FreeStyleProject: Jelly index: hudson/model/Job/index.jelly",
    "hudson.model.FreeStyleProject: StaplerProxy.getTarget()",
    "hudson.model.Hudson#doCheckDisplayName",
    "hudson.model.Hudson#doConfigSubmit",
    "hudson.model.Hudson#doResources",
    "hudson.model.Hudson#doScript",
    "hudson.model.Hudson#getAdjuncts(String)",
    "hudson.model.Hudson#getDescriptorByName(String)",
    "hudson.model.Hudson#getDynamic(...)",
    "hudson.model.Hudson#getJob(String)",
    "hudson.model.Hudson#getLog()",
    "hudson.model.Hudson#getPlugin(String)",
    "hudson.model.Hudson: Dispatch",
    "hudson.model.Hudson: Jelly facet: jenkins/model/Jenkins/configure.jelly",
    "hudson.model.Hudson: StaplerFallback.getStaplerFallback()",
    "hudson.model.Hudson: StaplerProxy.getTarget()",
    "hudson.plugins.git.GitSCM$DescriptorImpl#doFillGitToolItems",
    "hudson.plugins.git.GitSCM$DescriptorImpl: Dispatch",
    "hudson.plugins.git.GitTool$DescriptorImpl#doCheckName",
    "hudson.plugins.git.GitTool$DescriptorImpl: Dispatch",
    "hudson.plugins.git.UserRemoteConfig$DescriptorImpl#doCheckUrl",
    "hudson.plugins.git.UserRemoteConfig$DescriptorImpl#doFillCredentialsIdItems",
    "hudson.plugins.git.UserRemoteConfig$DescriptorImpl: Dispatch",
    "hudson.plugins.mercurial.MercurialSCM$DescriptorImpl#doFillCredentialsIdItems",
    "hudson.plugins.mercurial.MercurialSCM$DescriptorImpl: Dispatch",
    "hudson.plugins.throttleconcurrents.ThrottleJobProperty$DescriptorImpl#doCheckMaxConcurrentPerNode",
    "hudson.plugins.throttleconcurrents.ThrottleJobProperty$DescriptorImpl#doCheckMaxConcurrentTotal",
    "hudson.plugins.throttleconcurrents.ThrottleJobProperty$DescriptorImpl: Dispatch",
    "hudson.security.GlobalSecurityConfiguration#doConfigure",
    "hudson.security.GlobalSecurityConfiguration: Dispatch",
    "hudson.security.ProjectMatrixAuthorizationStrategy$2#doCheckName",
    "hudson.security.ProjectMatrixAuthorizationStrategy$2: Dispatch",
    "hudson.triggers.SCMTrigger$DescriptorImpl#doCheckScmpoll_spec",
    "hudson.triggers.SCMTrigger$DescriptorImpl: Dispatch",
    "hudson.triggers.TimerTrigger$DescriptorImpl#doCheckSpec",
    "hudson.triggers.TimerTrigger$DescriptorImpl: Dispatch",
    "hudson.widgets.BuildHistoryWidget#doAjax",
    "hudson.widgets.BuildHistoryWidget: Dispatch",
    "hudson.widgets.RenderOnDemandClosure#render",
    "hudson.widgets.RenderOnDemandClosure: Dispatch",
    "io.jenkins.blueocean.BlueOceanUI#getDynamic(...)",
    "io.jenkins.blueocean.BlueOceanUI: Dispatch",
    "io.jenkins.blueocean.BlueOceanUI: Jelly index: io/jenkins/blueocean/BlueOceanUI/index.jelly",
    "io.jenkins.blueocean.i18n.BlueI18n#doDynamic(...)",
    "io.jenkins.blueocean.i18n.BlueI18n: Dispatch",
    "io.jenkins.blueocean.rest.ApiHead#getDynamic(...)",
    "io.jenkins.blueocean.rest.ApiHead: Dispatch",
    "io.jenkins.blueocean.rest.impl.pipeline.PipelineImpl#getRuns()",
    "io.jenkins.blueocean.rest.impl.pipeline.PipelineImpl: Dispatch",
    "io.jenkins.blueocean.rest.impl.pipeline.PipelineNodeContainerImpl: Dispatch",
    "io.jenkins.blueocean.rest.impl.pipeline.PipelineNodeContainerImpl: Index: list",
    "io.jenkins.blueocean.rest.impl.pipeline.PipelineRunImpl#getArtifacts()",
    "io.jenkins.blueocean.rest.impl.pipeline.PipelineRunImpl#getBlueTestSummary()",
    "io.jenkins.blueocean.rest.impl.pipeline.PipelineRunImpl#getLog()",
    "io.jenkins.blueocean.rest.impl.pipeline.PipelineRunImpl#getNodes()",
    "io.jenkins.blueocean.rest.impl.pipeline.PipelineRunImpl#getSteps()",
    "io.jenkins.blueocean.rest.impl.pipeline.PipelineRunImpl#replay",
    "io.jenkins.blueocean.rest.impl.pipeline.PipelineRunImpl: Dispatch",
    "io.jenkins.blueocean.rest.impl.pipeline.PipelineStepContainerImpl: Dispatch",
    "io.jenkins.blueocean.rest.impl.pipeline.PipelineStepContainerImpl: Index: list",
    "io.jenkins.blueocean.rest.model.BlueTestSummary: Dispatch",
    "io.jenkins.blueocean.rest.model.BlueTestSummary: Index: getState",
    "io.jenkins.blueocean.service.embedded.BlueOceanRootAction: Dispatch",
    "io.jenkins.blueocean.service.embedded.BlueOceanRootAction: StaplerProxy.getTarget()",
    "io.jenkins.blueocean.service.embedded.rest.ArtifactContainerImpl: Dispatch",
    "io.jenkins.blueocean.service.embedded.rest.ArtifactContainerImpl: Index: list",
    "io.jenkins.blueocean.service.embedded.rest.ExtensionClassContainerImpl#getDynamic(...)",
    "io.jenkins.blueocean.service.embedded.rest.ExtensionClassContainerImpl: Dispatch",
    "io.jenkins.blueocean.service.embedded.rest.ExtensionClassContainerImpl: Index: getMap",
    "io.jenkins.blueocean.service.embedded.rest.ExtensionClassImpl: Dispatch",
    "io.jenkins.blueocean.service.embedded.rest.ExtensionClassImpl: Index: getState",
    "io.jenkins.blueocean.service.embedded.rest.FavoriteContainerImpl: Dispatch",
    "io.jenkins.blueocean.service.embedded.rest.FavoriteContainerImpl: Index: list",
    "io.jenkins.blueocean.service.embedded.rest.LogResource: Dispatch",
    "io.jenkins.blueocean.service.embedded.rest.LogResource: Index: doIndex",
    "io.jenkins.blueocean.service.embedded.rest.OrganizationContainerImpl#getDynamic(...)",
    "io.jenkins.blueocean.service.embedded.rest.OrganizationContainerImpl: Dispatch",
    "io.jenkins.blueocean.service.embedded.rest.OrganizationImpl#getPipelines()",
    "io.jenkins.blueocean.service.embedded.rest.OrganizationImpl#getUsers()",
    "io.jenkins.blueocean.service.embedded.rest.OrganizationImpl: Dispatch",
    "io.jenkins.blueocean.service.embedded.rest.PipelineContainerImpl#getDynamic(...)",
    "io.jenkins.blueocean.service.embedded.rest.PipelineContainerImpl: Dispatch",
    "io.jenkins.blueocean.service.embedded.rest.RunContainerImpl#getDynamic(...)",
    "io.jenkins.blueocean.service.embedded.rest.RunContainerImpl: Dispatch",
    "io.jenkins.blueocean.service.embedded.rest.RunContainerImpl: Index: list",
    "io.jenkins.blueocean.service.embedded.rest.UserContainerImpl#getDynamic(...)",
    "io.jenkins.blueocean.service.embedded.rest.UserContainerImpl: Dispatch",
    "io.jenkins.blueocean.service.embedded.rest.UserImpl#getFavorites()",
    "io.jenkins.blueocean.service.embedded.rest.UserImpl: Dispatch",
    "jenkins.branch.RateLimitBranchProperty$JobPropertyImpl$DescriptorImpl#doCheckCount",
    "jenkins.branch.RateLimitBranchProperty$JobPropertyImpl$DescriptorImpl#doFillDurationNameItems",
    "jenkins.branch.RateLimitBranchProperty$JobPropertyImpl$DescriptorImpl: Dispatch",
    "jenkins.model.AssetManager#doDynamic(...)",
    "jenkins.model.AssetManager: Dispatch",
    "jenkins.model.JenkinsLocationConfiguration#doCheckUrl",
    "jenkins.model.JenkinsLocationConfiguration: Dispatch",
    "jenkins.model.ProjectNamingStrategy$PatternProjectNamingStrategy$DescriptorImpl#doCheckNamePattern",
    "jenkins.model.ProjectNamingStrategy$PatternProjectNamingStrategy$DescriptorImpl: Dispatch",
    "jenkins.tools.GlobalToolConfiguration: Dispatch",
    "jenkins.triggers.ReverseBuildTrigger$DescriptorImpl#doCheckUpstreamProjects",
    "jenkins.triggers.ReverseBuildTrigger$DescriptorImpl: Dispatch",
    "org.jenkins.plugins.lockableresources.LockableResources#doDynamic(...)",
    "org.jenkins.plugins.lockableresources.LockableResources: Dispatch",
    "org.jenkins.plugins.lockableresources.RequiredResourcesProperty$DescriptorImpl#doCheckLabelName",
    "org.jenkins.plugins.lockableresources.RequiredResourcesProperty$DescriptorImpl#doCheckResourceNames",
    "org.jenkins.plugins.lockableresources.RequiredResourcesProperty$DescriptorImpl#doCheckResourceNumber",
    "org.jenkins.plugins.lockableresources.RequiredResourcesProperty$DescriptorImpl: Dispatch",
    "org.jenkinsci.plugins.docker.commons.credentials.DockerRegistryEndpoint$DescriptorImpl#doFillCredentialsIdItems",
    "org.jenkinsci.plugins.docker.commons.credentials.DockerRegistryEndpoint$DescriptorImpl: Dispatch",
    "org.jenkinsci.plugins.github.GitHubPlugin#doDynamic(...)",
    "org.jenkinsci.plugins.github.GitHubPlugin: Dispatch",
    "org.jenkinsci.plugins.github.config.GitHubPluginConfig#doCheckHookUrl",
    "org.jenkinsci.plugins.github.config.GitHubPluginConfig: Dispatch",
    "org.jenkinsci.plugins.github.config.HookSecretConfig$DescriptorImpl#doFillCredentialsIdItems",
    "org.jenkinsci.plugins.github.config.HookSecretConfig$DescriptorImpl: Dispatch",
    "org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SecureGroovyScript$DescriptorImpl#doCheckScript",
    "org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.SecureGroovyScript$DescriptorImpl: Dispatch",
    "org.jenkinsci.plugins.ssegateway.Endpoint#doConfigure",
    "org.jenkinsci.plugins.ssegateway.Endpoint#doConnect",
    "org.jenkinsci.plugins.ssegateway.Endpoint: Dispatch",
    "org.kohsuke.stapler.bind.BoundObjectTable$Table#getDynamic(...)",
    "org.kohsuke.stapler.bind.BoundObjectTable$Table: Dispatch",
    "org.kohsuke.stapler.bind.BoundObjectTable: Dispatch",
    "org.kohsuke.stapler.bind.BoundObjectTable: StaplerFallback.getStaplerFallback()",
    "org.kohsuke.stapler.framework.adjunct.AdjunctManager#doDynamic(...)",
    "org.kohsuke.stapler.framework.adjunct.AdjunctManager: Dispatch"
  ]
}

Proposed changelog entries

  • Add telemetry trial for Stapler web request dispatches

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • [n/a] For dependency updates: links to external changelogs and, if possible, full diffs
private Object buildComponentInformation() {
Map<String, String> components = new TreeMap<>();
VersionNumber core = Jenkins.getVersion();
components.put("jenkins-core", core == null ? "" : core.toString());

This comment has been minimized.

Copy link
@rtyler

rtyler Oct 10, 2018

Member

How would it be possible for Jenkins.getVersion to respond with null? 😕

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Oct 10, 2018

Author Member

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;

This comment has been minimized.

Copy link
@rtyler

rtyler Oct 10, 2018

Member

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 🤷‍♂️

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Oct 10, 2018

Author Member

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.

This comment has been minimized.

Copy link
@rtyler

rtyler Oct 10, 2018

Member

Wacky, never knew that, thanks!


@Override
protected void record(StaplerRequest staplerRequest, String s) {
if (UsageStatistics.DISABLED || !Jenkins.get().isUsageStatisticsCollected()) {

This comment has been minimized.

Copy link
@rtyler

rtyler Oct 10, 2018

Member

I'm assuming this is just an optimization, since Telemetry is not reported regardless when these settings are present, correct?

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Oct 10, 2018

Author Member

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.

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Oct 10, 2018

Author Member

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.

This comment has been minimized.

Copy link
@jglick

jglick Oct 11, 2018

Member

Please add a TODO comment here reminding people to use that API once merged.

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Oct 11, 2018

Author Member

Done.

// do not collect traces while usage statistics are disabled
return;
}
traces.add(s);

This comment has been minimized.

Copy link
@rtyler

rtyler Oct 10, 2018

Member

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 😸

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Oct 10, 2018

Author Member

Essentially done by replacing the set in https://github.com/jenkinsci/jenkins/pull/3688/files#diff-db538ffb33f05cbc1d77afb1fe990dccR88 and letting gc do the rest.

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Oct 10, 2018

Author Member

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.

This comment has been minimized.

Copy link
@rtyler
@jvz

jvz approved these changes Oct 10, 2018

@jglick

jglick approved these changes Oct 11, 2018


for (PluginWrapper plugin : Jenkins.get().pluginManager.getPlugins()) {
if (plugin.isActive()) {
components.put(plugin.getShortName(), plugin.getVersion());

This comment has been minimized.

Copy link
@jglick

jglick Oct 11, 2018

Member

This seems like boilerplate. Surely it should just be pulled up into Telemetry?

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Oct 11, 2018

Author Member

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()) {

This comment has been minimized.

Copy link
@jglick

jglick Oct 11, 2018

Member

Please add a TODO comment here reminding people to use that API once merged.

daniel-beck added some commits Oct 11, 2018

@jglick
Copy link
Member

jglick left a comment

minor stylistic issues


private Object buildDispatches() {
Set<String> currentTraces = traces;
traces = new TreeSet<>();

This comment has been minimized.

Copy link
@jglick

jglick Oct 11, 2018

Member

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.

This comment has been minimized.

Copy link
@jglick

jglick Oct 12, 2018

Member

Does not likely matter much, just FYI. Generally avoid volatile.

}
}

private static volatile Set<String> traces = new TreeSet<>();

This comment has been minimized.

Copy link
@jglick

jglick Oct 11, 2018

Member

Should be an instance variable, not static, and accessed from StaplerTrace via ExtensionList.lookupSingleton.

This comment has been minimized.

Copy link
@jglick

jglick Oct 24, 2018

Member

Lack of synchronization led to #3700 FTR.

@daniel-beck daniel-beck merged commit d1cbb20 into jenkinsci:master Oct 12, 2018

1 check was pending

continuous-integration/jenkins/pr-merge This commit is being built
Details

olivergondza added a commit that referenced this pull request Oct 23, 2018

[JENKINS-54029] Add telemetry for Stapler dispatches (#3688)
* Add telemetry for Stapler dispatches

* Add TODO

* Released version of Stapler

(cherry picked from commit d1cbb20)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.