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

Traces custom source - Bug Fixes #2298

Merged
merged 11 commits into from
Jan 8, 2025

Conversation

TackAdam
Copy link
Collaborator

@TackAdam TackAdam commented Dec 21, 2024

Description

  1. Handle a bug that was causing a page crash if the filter was selected twice.
  2. Now removes the filter when clicking.
  3. Set a default metric when page loading for custom source. (Was defaulting to no selection)
  4. Handle detection of start time by distinguishing between mili and nano seconds based on the sort field length.
  5. Added toast messages for 503 returns from traces and services request handlers so the UI reflects issues.
  6. Added trace-mode to the url so that new tabs and re-directs/favorites will work. (Previously it defaulted to data_prepper causing jaeger and custom_data_prepper links to break as session storage was not available in the new tabs)

Before: (Bug crashing page when a selected filter was clicked again)

Before.mov

After: (No longer crashes, now removes the filter when clicked again)

After.mov

Before: (When the custom traces sources used date for the start time mapping instead of date_nanos the times would all start at 0)
Before1

After: (Now the time conversion is determined by the length of the sort metric. This example uses the mapping of start time "date", the indexes themselves have some in mili seconds and some in nanos but because the mapping of "date" is used the nanosecond precision is lost.)
After1

Toast message example (manually setting buckets to 1 to cause failure)

ToastMessage.mov

URL redirection working:

UrlTest.mov

Issues Resolved

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@TackAdam TackAdam added bug Something isn't working backport 2.x labels Jan 2, 2025
@TackAdam TackAdam marked this pull request as ready for review January 6, 2025 19:42
@TackAdam TackAdam changed the title [WIP]Traces custom source - Bug Fixes Traces custom source - Bug Fixes Jan 6, 2025
@@ -62,6 +62,17 @@ export function DataSourcePicker(props: {
);
};

const updateUrlWithMode = (key: TraceAnalyticsMode) => {
const currentUrl = window.location.href.split('?')[0];
const queryParams = new URLSearchParams(window.location.search);
Copy link
Member

Choose a reason for hiding this comment

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

this will only get the standard search params, but not the search param we use with hash router. for example in playground https://playground.opensearch.org/app/observability-traces#/traces?datasourceId=44cc4c50-6f80-11ed-a8ec-958f39714b93

window.location.search is '', and currentUrl is 'https://playground.opensearch.org/app/observability-traces#/traces'. The dataSourceId information becomes lost?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated too


  const updateUrlWithMode = (key: TraceAnalyticsMode) => {
    const currentUrl = window.location.href.split('#')[0];
    const hash = window.location.hash;
  
    if (hash) {
      const [hashPath, hashQueryString] = hash.split('?');
      const queryParams = new URLSearchParams(hashQueryString || '');
      queryParams.set('mode', key);

      const newHash = `${hashPath}?${queryParams.toString()}`;
      const newUrl = `${currentUrl}#${newHash}`;
      window.history.replaceState(null, '', newUrl);
    } else {
      //Non-hash-based URL
      const queryParams = new URLSearchParams(window.location.search);
      queryParams.set('mode', key);
  
      const newUrl = `${currentUrl}?${queryParams.toString()}`;
      window.history.replaceState(null, '', newUrl);
    }
  };

);
const renderTraceLinkField = (item: string) => {
// Extract the current mode from the URL or session storage
const queryParams = new URLSearchParams(window.location.search);
Copy link
Member

Choose a reason for hiding this comment

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

does this work correctly with hash router?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated too

const currentUrl = window.location.href;
const traceMode = new URLSearchParams(currentUrl.split('?')[1]).get('mode') || sessionStorage.getItem('TraceAnalyticsMode');
export const appendModeToTraceViewUri = (
  traceId: string,
  getTraceViewUri?: (traceId: string) => string,
  traceMode?: string | null
): string => {
  const baseUri = getTraceViewUri ? getTraceViewUri(traceId) : '';
  const isHashRouter = baseUri.includes('#');
  const separator = isHashRouter ? (baseUri.includes('?') ? '&' : '?') : (baseUri.includes('?') ? '&' : '?');

  if (traceMode) {
    return `${baseUri}${separator}mode=${encodeURIComponent(traceMode)}`;
  }
  return baseUri;
};

return `${baseUri}${separator}mode=${encodeURIComponent(traceMode)}`;
}
return baseUri;
};
Copy link
Member

Choose a reason for hiding this comment

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

this logic is being duplicated, can it be extracted? might also need unit tests

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved to public/components/trace_analytics/components/common/helper_functions.tsx

Added unit test

  describe('getTimestampPrecision', () => {
    it('returns "millis" for 13-digit timestamps', () => {
      expect(getTimestampPrecision(1703599200000)).toEqual('millis');
    });
  
    it('returns "micros" for 16-digit timestamps', () => {
      expect(getTimestampPrecision(1703599200000000)).toEqual('micros');
    });
  
    it('returns "nanos" for 19-digit timestamps', () => {
      expect(getTimestampPrecision(1703599200000000000)).toEqual('nanos');
    });
  
    it('returns "millis" for invalid or missing timestamps', () => {
      expect(getTimestampPrecision(undefined as unknown as number)).toEqual('millis');
      expect(getTimestampPrecision(123)).toEqual('millis');
    });
  });
  
  describe('appendModeToTraceViewUri', () => {
    const mockGetTraceViewUri = (traceId: string) => `#/traces/${traceId}`;
  
    it('appends mode to the URI when mode is provided', () => {
      const result = appendModeToTraceViewUri('123', mockGetTraceViewUri, 'data_prepper');
      expect(result).toEqual('#/traces/123?mode=data_prepper');
    });
  
    it('appends mode correctly for hash router URIs with existing query params', () => {
      const result = appendModeToTraceViewUri('123', (id) => `#/traces/${id}?foo=bar`, 'jaeger');
      expect(result).toEqual('#/traces/123?foo=bar&mode=jaeger');
    });
  
    it('does not append mode if not provided', () => {
      const result = appendModeToTraceViewUri('123', mockGetTraceViewUri, null);
      expect(result).toEqual('#/traces/123');
    });
  
    it('handles URIs without a hash router', () => {
      const result = appendModeToTraceViewUri('123', (id) => `/traces/${id}`, 'custom_data_prepper');
      expect(result).toEqual('/traces/123?mode=custom_data_prepper');
    });
  
    it('handles URIs without a hash router and existing query params', () => {
      const result = appendModeToTraceViewUri('123', (id) => `/traces/${id}?foo=bar`, 'jaeger');
      expect(result).toEqual('/traces/123?foo=bar&mode=jaeger');
    });
  }); 

// if (mode === 'jaeger' && !response?.aggregations?.service_type?.buckets) {
// console.warn('No traces or aggregations found.');
// return [];
// }
Copy link
Member

Choose a reason for hiding this comment

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

is this needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed, became unnecessary after the url mode implementation.

const traceMode = queryParams.get('mode') || sessionStorage.getItem('TraceAnalyticsMode');

// Update the href to include the mode if available
const updatedGetTraceViewUri = (traceId: string) => {
Copy link
Member

Choose a reason for hiding this comment

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

"update get trace view uri" is not very clear, maybe

Suggested change
const updatedGetTraceViewUri = (traceId: string) => {
const appendModeToTraceViewUri = (traceId: string) => {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated, thank you.

Signed-off-by: Adam Tackett <[email protected]>
Adam Tackett added 3 commits January 7, 2025 15:29
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
@TackAdam TackAdam merged commit a57c10e into opensearch-project:main Jan 8, 2025
17 of 18 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 8, 2025
* fix custom traces filter bug, add default metric selection

Signed-off-by: Adam Tackett <[email protected]>

* helper function for start time, handle date and datenanos

Signed-off-by: Adam Tackett <[email protected]>

* add toast messages for all errors service-traces, add mode to url

Signed-off-by: Adam Tackett <[email protected]>

* fix url re-direction in non custom state

Signed-off-by: Adam Tackett <[email protected]>

* address comments, add testing

Signed-off-by: Adam Tackett <[email protected]>

* update url #, send mode to applications

Signed-off-by: Adam Tackett <[email protected]>

* update helper function

Signed-off-by: Adam Tackett <[email protected]>

* add validation to url mode

Signed-off-by: Adam Tackett <[email protected]>

* fix applications removing traces redirect

Signed-off-by: Adam Tackett <[email protected]>

---------

Signed-off-by: Adam Tackett <[email protected]>
Co-authored-by: Adam Tackett <[email protected]>
(cherry picked from commit a57c10e)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
paulstn pushed a commit that referenced this pull request Jan 8, 2025
* fix custom traces filter bug, add default metric selection



* helper function for start time, handle date and datenanos



* add toast messages for all errors service-traces, add mode to url



* fix url re-direction in non custom state



* address comments, add testing



* update url #, send mode to applications



* update helper function



* add validation to url mode



* fix applications removing traces redirect



---------



(cherry picked from commit a57c10e)

Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Adam Tackett <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants