tracing: Replace Provisioning mocks with actual API calls

changed milestone to %16.3

assigned to @drosse

3 Warnings

⚠

: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines.

⚠

: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines.

⚠

featureaddition and featureenhancement merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the Technical Writer counterpart.

For more information, see:

The Handbook page on merge request types.
The definition of done documentation.

1 Message

📖

CHANGELOG missing:

If you want to create a changelog entry for GitLab FOSS, add the Changelog trailer to the commit message you want to add to the changelog.

If you want to create a changelog entry for GitLab EE, also add the EE: true trailer to your commit message.

If this merge request doesn't need a CHANGELOG entry, feel free to ignore this message.

Reviewer roulette

Changes that require review have been detected!

Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:

Category	Reviewer	Maintainer
backend	Martin Cavoj (`@mcavoj`) (UTC+2, 1 hour ahead of `@drosse`)	Alex Pooley (`@alexpooley`) (UTC+8, 7 hours ahead of `@drosse`)
frontend	Scott de Jonge (`@sdejonge`) (UTC+10, 9 hours ahead of `@drosse`)	Miguel Rincon (`@mrincon`) (UTC+2, 1 hour ahead of `@drosse`)

To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.

To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.

Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.

If needed, you can retry the 🔁 danger-review job that generated this comment.

Generated by 🚫 Danger

Proper labels assigned to this merge request. Please ignore me.

👋 @drosse - please see the following guidance and update this merge request.

	1 Error
❌	Please add typebug typefeature, or typemaintenance label to this merge request.

Bundle size analysis [beta]

This compares changes in bundle size for entry points between the commits f74d84e2 and 07c4685b

✨ Special assets

Entrypoint / Name	Size before	Size after	Diff	Diff in percent
average	4.17 MB	4.17 MB	-	0.0 %
mainChunk	3.02 MB	3.02 MB	-	0.0 %

Note: We do not have exact data for f74d84e2. So we have used data from: 6dffb18a.
The intended commit has no webpack pipeline, so we chose the last commit with one before it.

Please look at the full report for more details

Allure report

allure-report-publisher generated test report!

e2e-test-on-gdk: ❗ test report for 07c4685b

expand test summary

+-----------------------------------------------------------------------+
|                            suites summary                             |
+------------------+--------+--------+---------+-------+-------+--------+
|                  | passed | failed | skipped | flaky | total | result |
+------------------+--------+--------+---------+-------+-------+--------+
| Plan             | 51     | 0      | 0       | 0     | 51    | ✅     |
| Create           | 46     | 0      | 1       | 2     | 47    | ❗     |
| Manage           | 13     | 0      | 1       | 1     | 14    | ❗     |
| Govern           | 36     | 0      | 0       | 0     | 36    | ✅     |
| Framework sanity | 0      | 0      | 1       | 0     | 1     | ➖     |
| Monitor          | 4      | 0      | 0       | 0     | 4     | ✅     |
| Data Stores      | 22     | 0      | 0       | 0     | 22    | ✅     |
| Verify           | 8      | 0      | 0       | 0     | 8     | ✅     |
| Package          | 0      | 0      | 1       | 0     | 1     | ➖     |
+------------------+--------+--------+---------+-------+-------+--------+
| Total            | 180    | 0      | 4       | 3     | 184   | ❗     |
+------------------+--------+--------+---------+-------+-------+--------+

e2e-review-qa: ❗ test report for 07c4685b

expand test summary

+-----------------------------------------------------------------------+
|                            suites summary                             |
+------------------+--------+--------+---------+-------+-------+--------+
|                  | passed | failed | skipped | flaky | total | result |
+------------------+--------+--------+---------+-------+-------+--------+
| Framework sanity | 0      | 0      | 1       | 0     | 1     | ➖     |
| Plan             | 3      | 0      | 1       | 0     | 4     | ✅     |
| Monitor          | 4      | 0      | 0       | 1     | 4     | ❗     |
| Create           | 8      | 0      | 1       | 0     | 9     | ✅     |
| Manage           | 1      | 0      | 0       | 0     | 1     | ✅     |
| Govern           | 2      | 0      | 0       | 0     | 2     | ✅     |
| Data Stores      | 2      | 0      | 0       | 0     | 2     | ✅     |
| Package          | 0      | 0      | 1       | 0     | 1     | ➖     |
+------------------+--------+--------+---------+-------+-------+--------+
| Total            | 20     | 0      | 4       | 1     | 24    | ❗     |
+------------------+--------+--------+---------+-------+-------+--------+

changed the description

Setting label groupobservability based on @drosse's group.

added groupobservability label

added 1 commit

42cb434e - tracing: Remove Provisioning API mocks

Compare with previous version

added devopsanalyze sectionanalytics labels

added 1 commit

b7b87c04 - tracing: Remove Provisioning API mocks

Compare with previous version

changed title from Draft: tracing: Remove Provisioning API mocks to Draft: tracing: Replace Provisioning mocks with actual API calls

marked the checklist item I have evaluated the MR acceptance checklist for this MR. as completed

added featureaddition label

added typefeature label

requested review from @mcavoj

requested review from @sdejonge

removed review request for @sdejonge

Hi @mcavoj any chance you can review the 1-line change in backend ? Thanks :) 🙏

requested review from @robyrne

marked this merge request as ready

changed the description

requested review from @alexpooley and removed review request for @mcavoj

approved this merge request

👋 @mcavoj, thanks for approving this merge request.

This is the first time the merge request is approved. To ensure full test coverage, a new pipeline will be started shortly.

For more info, please refer to the following links:

added pipeline:mr-approved label

mentioned in epic gitlab-org/opstrace&77 (closed)

approved this merge request

requested review from @mrincon and removed review request for @robyrne

Question: I am not too familiar with this area, why is this v3? It this a standard we can assume folks to know about?

If not it would be nice to have in a constant that describes where it comes from. 👍

This is api prefix currently used by observability backend - we are at version v3 ATM, v1 and v2 being previous iterations.

👍 If it is our own, is there a way to use Rails routes to generate it?

Note this API is not in Rails but a separate backend

good point - I think we can leveraging direct routes

See some of our starts with this in config/routes/directs.rb

What's the advantage of using direct routes in this case? Is it just to make the *_url available everywhere? Do we need/want to? I think having it encapsulated in the Observability module makes more sense. WDYT?

I don't have a strong opinion on using routes, but it would be nice to have a single source of truth for this. We have other urls with v1 so it's hard to see from looking at the code where the difference comes from.

it would be nice to have a single source of truth for this

This module GitLab::Observability is meant to be the single source of truth for Observability API. Or did you mean something differently?

Perhaps this is as simple as a constant in this module? I see v1 as well so we are supporting both.

Ah I see what you mean. I'm not sure how the backend versioning works tbh. Let me ask the backend team here.

@gl-observability Hey folks can you shed some lights here about backend versioning? Do different API have different versions? Or all share the same version?

For instance, we have the following URL defined here:

    def oauth_url
      "#{Gitlab::Observability.observability_url}/v1/auth/start"
    end

    def tracing_url(project)
      "#{Gitlab::Observability.observability_url}/query/#{project.group.id}/#{project.id}/v1/traces"
    end

    def provisioning_url(project)
      "#{Gitlab::Observability.observability_url}/v3/tenant/#{project.id}"
    end

Both v1 and v3 are used. Should they all be the same? Or are they not shared?

@drosse They should ideally be all the same (v3), but they're also not shared right now.

v3 primarily comes from our decision to build new APIs/endpoints as per the latest architectural changes deprecating GOUI-specific things which we drafted as v2.
The tracing_url was discussed to be changed to v3 here: gitlab-org/opstrace/opstrace!2146 (comment 1468395164) and being tracked with gitlab-org/opstrace/opstrace#2282.
The oauth_url with v1 can stay as is for now, independent of our backend/subsystem specific endpoints.

Thanks for the clarification @ankitbhatnagar !

@mrincon Based on the above, I'm not sure it makes sense to extract the version, as for the moment at least, each API is using is own version and there is maybe not much point in having 3 constants used by 3 APIs. WDYT?

Ok, as a reviewer it's difficult to know where all those versions came from but it looks like you can document this elsewhere, I won't block this MR over this. 👍

Thanks for the discussion here. I still think we should try to go in a direction of using direct routes for in house url definitions, but I won't block this and will spawn a follow-up issue instead.