Wrong root span name and value of the `ci.pipeline.id` attribute for traces of PR Builds on Multibranch Pipelines

[jenkinsci]

Wrong root span name and value of the `ci.pipeline.id` attribute for traces of PR Builds on Multibranch Pipelines

[kuisathaverat]

The pattern of the change request names depends on the SCM service, Github, Gerrit, or GitLab do not use the same pattern. So due to each SCM has its own rules to manage the change request names, I think that the best value for the ci.pipeline.id should be the name of the job path/to/Build/job/PR-123, in this way all builds of the same job are identified by the same ci.pipeline.id and we can group them. For the root span, I would suggest using the same value ci.pipeline.id for the same reason this allows us to identify and measure the whole spans sequence, the build number is an attribute, if someone needs it is possible to check it (BUILD_NUMBER).
Also, I'd not try to differentiate change requests and branches by the names of the spans, it is better to use attributes to classify the spans.

[cyrille-leclerc]

with pleasure @BerndFarkaDyna , the more contributors we have, the better :-)

[cyrille-leclerc]

changing the name of the span to the pipeline name.

Yes.
Question: according to the source code, the change request job name can be suffixed by -head or -merge. I've never seen these suffixes in "real life". Should we keep these suffixes? I guess that we could simplify and use the jobFullName

shouldn't be this done for branches to? (cardinality reduction?)

I would continue to have one root span name per branch, I feel it's aligned with the idea to have one root span name per change-request / pull-request. I feel the change you propose will better align with the existing naming strategy for branches.

fix the attribute ci.pipeline.id to contain the jobFullName.

Yes. From what I remember, it should be implicitly fixed changing the logic of the method SpanNamingStrategy#getChangeRequestRootSpanName().

Note that for a pipeline execution my-war/PR-2/1, we have:

• ci.pipeline.id: my-war/PR-2
• ci.pipeline.run.number: 1

[v1v]

Hi @BerndFarkaDyna, thanks for raising this.

That's intended and the convention we have added. The original implementation was done in #47

Cheers

[v1v]

I've been thinking about this, I see the benefits in reducing the cardinality, but I'd like to ask your needs for using Path/to/Build/job/PR-123/1?

@cyrille-leclerc, what do think about giving the flexibility in the JCasC configuration to opt-in for changing the convention to a high cardinality in the PRs on Multibranch Pipelines?

[cyrille-leclerc]

Thanks @BerndFarkaDyna for your feedback.

The value of ci.pipeline.id being path/to/Build/pr-{number} rather than path/to/Build/job/PR-123/1 looks definitively like a bug.

On the other hand, as @v1v said, we decided to pick a quite generic root span name like my-war/PR-{number} to have a low cardinality span name.

The pattern my-war/PR-{number} is the solution we picked to comply with the OpenTelemetry Specification for span names defined in opentelemetry-specification/specification/trace/api.md

The span name concisely identifies the work represented by the Span,
for example, an RPC method name, a function name,
or the name of a subtask or stage within a larger computation.
The span name SHOULD be the most general string that identifies a
(statistically) interesting class of Spans,
rather than individual Span instances while still being human-readable.
That is, "get_user" is a reasonable name, while "get_user/314159",
where "314159" is a user ID, is not a good name due to its high cardinality.
Generality SHOULD be prioritized over human-readability.

For example, here are potential span names for an endpoint that gets a
hypothetical account information:

Span Name	Guidance
get	Too general
get_account/42	Too specific
get_account	Good, and account_id=42 would make a nice Span attribute
get_account/{accountId}	Also good (using the "HTTP route")

@BerndFarkaDyna do you agree with this rationale to keep a low cardinality on the root span name?

@v1v said:
@cyrille-leclerc, what do think about giving the flexibility in the JCasC configuration to opt-in for changing the convention to a high cardinality in the PRs on Multibranch Pipelines?

@v1v I'm not excited by this idea to allo high cardinality span names because Distributed Tracing solutions are optimized for low cardinality span names. I'll be interested to get @BerndFarkaDyna 's feedback.

[BerndFarkaDyna]

@cyrille-leclerc:

The value of ci.pipeline.id being path/to/Build/pr-{number} rather than path/to/Build/job/PR-123/1 looks definitively like a bug.

THX, this fix would help a lot.

About the cardinality on the root span name: Of course I understand you explanation and it makes sense to me, but for me it seems inconsistent since branch-builds of the same project do not have such an handling. IMHO that is a bit strange and i would even vote for having the behavior between Branch and PR builds the same since (AFIAK) there is no attribute or other property where i could distinguish between Branch and PR builds.

[cyrille-leclerc]

About the cardinality on the root span name: Of course I understand you explanation and it makes sense to me, but for me it seems inconsistent since branch-builds of the same project do not have such an handling. IMHO that is a bit strange and i would even vote for having the behavior between Branch and PR builds the same since (AFIAK) there is no attribute or other property where i could distinguish between Branch and PR builds.

You are right that there is a lack of consistency having different "root span name strategies" for the branch builds and the PR builds.
I think my questionable assumption was that the branch name was low cardinality because branches where long lived when the PR where short lived and thus high cardinality.
In fact, my rationale doesn't work when developer create feature/bug branches on the main repository.

I'm wondering

• If we could have a kind of "rule based strategy" to use a generic name for the root span name like. The default list could look like:feature/** , bugfix/**, pr-**
• Another idea could be to say that PR numbers are not so high cardinality and that we can name path/to/Build/job/PR-123/1 as path/to/Build/job/PR-123, just removing the build number (e.g. 1 in our example).

@v1v , @kuisathaverat any suggestion?

[cyrille-leclerc]

@BerndFarkaDyna can you please explain us how you this naming is impacting you? is it related to the visualization of the list of traces grouped by their "root name"?

[BerndFarkaDyna]

@cyrille-leclerc : we are doing our own processing of spans so there the name PR-{number} simply was misleading, plus i had no other chance since, as mentioned above, ci.pipeline.id also has the wrong value.

AFAIK: a very simple suggest to have consistency reporting:

• if 'ci.pipeline.id' is correct
• why not use the name of the job as span name? so you can get rid of the whole cardinality and via the name of the span it would be easy to identify the job (not important if it is an branch or pr build).

[cyrille-leclerc]

I'm convinced by @BerndFarkaDyna 's proposal to use the Pull Request / Change Request pipeline name. IDeally with a configuration entry to specify the naming strategy but we can start hardcoding this new strategy as a replacement to the strategy I initially implemented.

I may not have the time to propose a PR before my PTO and I'll be back on Aug 16th.

The code to change is:

https://github.com/jenkinsci/opentelemetry-plugin/blob/opentelemetry-0.16/src/main/java/io/jenkins/plugins/opentelemetry/job/SpanNamingStrategy.java#L54-L77

    @VisibleForTesting
    @Nonnull
    protected String getChangeRequestRootSpanName(@Nonnull String jobFullName) {
        // org.jenkinsci.plugins.github_branch_source.PullRequestGHEventSubscriber
        // com.cloudbees.jenkins.plugins.bitbucket.BitbucketSCMSource
        // jenkins.scm.api.mixin.ChangeRequestCheckoutStrategy
        // e.g. "my-war/PR-2", "my-war/PR-2-merge", "my-war/PR-2-head"
        // io.jenkins.plugins.gitlabbranchsource.GitLabSCMSource
        // e.g. "my-war/MR-2", "my-war/MR-2-merge", "my-war/MR-2-head"
        String rootSpanName = jobFullName;
        for (String changeRequestJobNameSuffix : CHANGE_REQUEST_JOB_NAME_SUFFIXES) {
            if (rootSpanName.endsWith(changeRequestJobNameSuffix)) {
                rootSpanName = rootSpanName.substring(0, rootSpanName.length() - changeRequestJobNameSuffix.length());
                break;
            }
        }

        // remove digits
        while (Character.isDigit(rootSpanName.charAt(rootSpanName.length() - 1))) {
            rootSpanName = rootSpanName.substring(0, rootSpanName.length() - 1);
        }
        if ('-' == rootSpanName.charAt(rootSpanName.length() - 1)) {
            return rootSpanName + "{number}";
        }
        return jobFullName;
    }

[BerndFarkaDyna]

@cyrille-leclerc : if you don't mind volunteer for creating this PR...

[BerndFarkaDyna]

Just to sum it up what the pr should include:

• changing the name of the span to the pipeline name.
• shouldn't be this done for branches to? (cardinality reduction?)
• fix the attribute 'ci.pipeline.id' to contain the jobFullName.