Skip to content

Conversation

iypetrov
Copy link

@iypetrov iypetrov commented Sep 5, 2025

Which problem is this PR solving?

Extends #3002
Previously Deep Dependencies view was always using /analytics/v1/deep-dependencies API endpoint to retrieve the data. Now this endpoint is configurable via UI config, but the default is changed to /api/deep-dependencies to match the change in the backend jaegertracing/jaeger#6606 that returns dummy data for said endpoint.

Part of jaegertracing/jaeger#7399.

Description of the changes

How was this change tested?

  • Unit tests

Checklist

sujalshah-bit and others added 3 commits July 30, 2025 12:10
This commit updates the Deep Dependency Graph UI logic
to use the correct backend API endpoint for fetching data.

Resolves [#6606](jaegertracing/jaeger#6606).

Part of [#7399](jaegertracing/jaeger#7399).

Signed-off-by: Sujal Shah <sujalshah28092004@gmail.com>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Ilia Petrov <ilia.yavorov.petrov@gmail.com>
@iypetrov iypetrov requested a review from a team as a code owner September 5, 2025 15:20
@iypetrov iypetrov requested review from albertteoh and removed request for a team September 5, 2025 15:20
Copy link
Contributor

graphite-app bot commented Sep 5, 2025

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

@@ -81,7 +81,7 @@ function getJSON(url, options = {}) {
}

export const DEFAULT_API_ROOT = prefixUrl('/api/');
export const ANALYTICS_ROOT = prefixUrl('/analytics/');
export const DEEP_DEPENDENCIES_ROOT = prefixUrl(getConfig().deepDependencies?.apiEndpoint);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of ? indicates the value might be null, why should it be allowed?

@@ -195,6 +195,8 @@ export type Config = {

deepDependencies?: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand you're just copying preexisting pattern, but I wonder why these need to be optional if our default config will always overwrite them if not defined? It seems not using optional will make the rest of the code simpler.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR configures the Deep Dependencies API endpoint to use a new configurable path instead of the hardcoded analytics endpoint. The default endpoint is changed from /analytics/v1/deep-dependencies to /api/deep-dependencies to align with backend changes that provide dummy data for this endpoint.

  • Adds configurable apiEndpoint and menuLabel options to the deepDependencies configuration
  • Updates the default configuration to use /api/deep-dependencies as the new endpoint
  • Refactors the API implementation to use the configurable endpoint instead of hardcoded analytics path

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
packages/jaeger-ui/src/types/config.tsx Adds type definitions for new configurable deep dependencies options
packages/jaeger-ui/src/constants/default-config.tsx Sets default values for the new deep dependencies configuration
packages/jaeger-ui/src/api/jaeger.js Updates API implementation to use configurable endpoint instead of hardcoded analytics path
packages/jaeger-ui/src/api/jaeger.test.js Updates test to reflect the new endpoint constant name

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@@ -81,7 +81,7 @@ function getJSON(url, options = {}) {
}

export const DEFAULT_API_ROOT = prefixUrl('/api/');
export const ANALYTICS_ROOT = prefixUrl('/analytics/');
export const DEEP_DEPENDENCIES_ROOT = prefixUrl(getConfig().deepDependencies?.apiEndpoint);
Copy link
Preview

Copilot AI Sep 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getConfig().deepDependencies?.apiEndpoint could be undefined if the configuration is not properly initialized, which would cause prefixUrl(undefined) to be called. This should have a fallback value or be wrapped in a function to ensure the config is available when accessed.

Suggested change
export const DEEP_DEPENDENCIES_ROOT = prefixUrl(getConfig().deepDependencies?.apiEndpoint);
export const DEEP_DEPENDENCIES_ROOT = prefixUrl(getConfig().deepDependencies?.apiEndpoint || '/api/dependencies');

Copilot uses AI. Check for mistakes.

Copy link

codecov bot commented Sep 5, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.13%. Comparing base (b375fa5) to head (16f16db).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3058   +/-   ##
=======================================
  Coverage   98.13%   98.13%           
=======================================
  Files         257      257           
  Lines        8004     8004           
  Branches     2014     2014           
=======================================
  Hits         7855     7855           
  Misses        149      149           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants