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

Fix: handle empty trace group and last updated values #445

Merged

Conversation

kevinwcyu
Copy link
Contributor

What this PR does / why we need it:
Fixes an issue where some trace groups and last updated values are empty. This causes a panic when parsing the returned trace from OpenSearch into a data frame field.

Which issue(s) this PR fixes:

Special notes for your reviewer:
You can test this by:

  1. Clone https://github.com/opensearch-project/opentelemetry-demo and run it with the following
    git clone https://github.com/opensearch-project/opentelemetry-demo.git
    cd opentelemetry-demo
    docker compose up -d
  2. Start your local Grafana instance on another port other than 3000
    [server]
    http_port = 4000
  3. Open http://localhost:4000 (use the port you set above) and create an opensearch data source with the following settings (password is my_%New%_passW0rd!@#)
    data source config
  4. Go to http://localhost:8089/ click Edit under the "Running" heading in the header and increase the Number of users and Spawn rate settings (I used 250 for Number of users and 50 for Spawn rate)
  5. Go to http://localhost:4000/explore set the time range to last 24 hours and run a Lucene Trace query (no need to fill in the query field).
  6. When you run OpenSearch without this PR it should have an An error occurred within the plugin error. It should return results with the trace group and last updated field blank when running OpenSearch with this PR.
    fixed
    Here's an image of the OpenSearch dashboard that shows that it leaves those fields blank
    opensearch

Note: I'm not sure exactly when a trace without a trace group or last updated value is generated, you may have to wait some time though.

@kevinwcyu kevinwcyu requested a review from a team as a code owner August 29, 2024 18:35
@kevinwcyu kevinwcyu requested review from njvrzm and nmarrs and removed request for a team August 29, 2024 18:35
@kevinwcyu kevinwcyu marked this pull request as draft August 29, 2024 18:49
Copy link
Contributor

@nmarrs nmarrs left a comment

Choose a reason for hiding this comment

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

I wasn't able to figure out local setup due to issues running the open telemetry demo locally :/ but code LGTM :) great job!

Copy link
Contributor

@njvrzm njvrzm left a comment

Choose a reason for hiding this comment

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

Looks good! One optional suggestion for a bit of efficiency & cleanup

pkg/opensearch/response_parser.go Outdated Show resolved Hide resolved
@kevinwcyu kevinwcyu merged commit fe2aaea into main Oct 2, 2024
4 checks passed
@kevinwcyu kevinwcyu deleted the kevinwcyu/handle-empty-trace-group-and-last-updated-traces branch October 2, 2024 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants