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

[jaeger-v2] add Badger storage backend integration test #5281

Closed

Conversation

Pushkarm029
Copy link
Member

Which problem is this PR solving?

Description of the changes

  • Added integration tests for Badger.
  • Modified GH Workflow to perform integration tests for jaeger-v2 also.

How was this change tested?

  • ./scripts/badger-integration-test.sh

Checklist

Signed-off-by: Pushkar Mishra <[email protected]>
@Pushkarm029
Copy link
Member Author

Pushkarm029 commented Mar 18, 2024

@james-ryans, this test is not able to receive data. any idea, how to tackle this?
log:

2024/03/18 12:02:01 Starting load generator at 1024 items/sec.
2024/03/18 12:02:03  | Sent:       539 traces (275/sec) | Received:         0 items (0/sec)
2024/03/18 12:02:06  | Sent:     1,533 traces (309/sec) | Received:         0 items (0/sec)
2024/03/18 12:02:08 Stopped generator. Sent:     1,533 traces (219/sec)
2024/03/18 12:02:09  | Sent:     1,533 traces (192/sec) | Received:         0 items (0/sec)
2024/03/18 12:02:12  | Sent:     1,533 traces (139/sec) | Received:         0 items (0/sec)
2024/03/18 12:02:15  | Sent:     1,533 traces (109/sec) | Received:         0 items (0/sec)
2024/03/18 12:02:18  | Sent:     1,533 traces (90/sec) | Received:         0 items (0/sec)
2024/03/18 12:02:18 Time out waiting for all data items received
    test_case.go:289: Time out waiting for all data items received

Copy link

codecov bot commented Mar 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.00%. Comparing base (53592b3) to head (2dfd9b9).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5281      +/-   ##
==========================================
- Coverage   95.08%   95.00%   -0.08%     
==========================================
  Files         340      340              
  Lines       16626    16626              
==========================================
- Hits        15809    15796      -13     
- Misses        628      642      +14     
+ Partials      189      188       -1     
Flag Coverage Δ
badger 12.74% <ø> (-0.52%) ⬇️
cassandra-3.x 26.44% <ø> (ø)
cassandra-4.x 26.44% <ø> (ø)
elasticsearch-5.x 21.70% <ø> (+0.01%) ⬆️
elasticsearch-6.x 21.68% <ø> (ø)
elasticsearch-7.x ?
elasticsearch-8.x ?
grpc 11.01% <ø> (+0.03%) ⬆️
kafka 14.73% <ø> (ø)
opensearch-1.x 21.77% <ø> (ø)
opensearch-2.x 21.77% <ø> (-0.02%) ⬇️
unittests 92.25% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@james-ryans
Copy link
Contributor

I've just learned that Badger is an embedded local storage. In Testbed, the collector and the data receiver are both running as separated instances. That being said the data receiver doesn't have access to anything inside the collector, that's why we can't test the memory storage backend. But from my perspective, it seems your Badger config writes and stores the spans in the /tmp/jaeger directory. Since both the collector and data receiver are running on the same computer, the data receiver should be able to read those spans from /tmp/jaeger directory. I'll try to take a look at how Badger works and update to you what I can do.

Also, that means you probably don't need to have a script to run a remote storage.

@yurishkuro
Copy link
Member

If you point two badger instances to the same file system they may run into synchronization issues. I don't think it's recommended.

The way to test badger and memory is by using jaeger remote storage, a binary that can run one instance to which you connect as grpc storage. I believe our crossdock tests are using that.

@james-ryans
Copy link
Contributor

If you point two badger instances to the same file system they may run into synchronization issues.

I think Yuri is right, I've just tried to run the Badger integration test with ephemeral: false in the config to read & write from the directory_key and directory_value. But I encountered this error.

panic: failed to start extensions: failed to initialize badger storage badger_main: Cannot acquire directory lock on "/tmp/jaeger".  Another process is using this Badger database. error: resource temporarily unavailable

The way to test badger and memory is by using jaeger remote storage

Yes, in the previous PR for gRPC integration test, I actually used memory span storage type for the remote storage. You may also follow that configuration at grpc_config.yaml. But for the script, we're actually trying to improve that by build a binary/container remote storage from "this" version of repository not the published image #5266.

@Pushkarm029 Pushkarm029 changed the title [jaeger-v2] add GRPC storage backend integration test [jaeger-v2] add Badger storage backend integration test Mar 18, 2024
@Pushkarm029
Copy link
Member Author

Thanks for the suggestions. I am trying it out, I will update you soon.

@Pushkarm029
Copy link
Member Author

Pushkarm029 commented Mar 19, 2024

Update: This method is working fine locally : sorry, it's not working locally also, throwing same dir error. it was working previously due to some misconfiguration of env variables.

However, replicating this in docker using a script throws this error because user_uid is set to 1001(non_root) in remote-storage.

"error":"Error Creating Dir: \"/go/bin/data/keys\" error: mkdir /go/bin/data: permission denied,"

Signed-off-by: Pushkar Mishra <[email protected]>
@yurishkuro
Copy link
Member

You don't want /go/bin as the data dir for badger. See #5282 about permissions

Signed-off-by: Pushkar Mishra <[email protected]>
Signed-off-by: Pushkar Mishra <[email protected]>
@Pushkarm029
Copy link
Member Author

Fixed this permission error, Still same :

2024/03/20 20:56:49 Starting load generator at 1024 items/sec.
2024/03/20 20:56:51  | Sent:       460 traces (231/sec) | Received:         0 items (0/sec)
2024/03/20 20:56:54  | Sent:     1,533 traces (307/sec) | Received:         0 items (0/sec)
2024/03/20 20:56:56 Stopped generator. Sent:     1,839 traces (268/sec)
2024/03/20 20:56:57  | Sent:     1,839 traces (230/sec) | Received:         0 items (0/sec)
2024/03/20 20:57:00  | Sent:     1,839 traces (167/sec) | Received:         0 items (0/sec)
2024/03/20 20:57:03  | Sent:     1,839 traces (131/sec) | Received:         0 items (0/sec)
2024/03/20 20:57:06  | Sent:     1,839 traces (108/sec) | Received:         0 items (0/sec)

Any other approach or fix @yashrsharma44

@Pushkarm029
Copy link
Member Author

dbentries.txt
I recovered the test database to verify that traces are stored in the remote badger database and tried to read key value pair. Got this result. Please check if it is the same as what we are supposed to store in it. @james-ryans

@yurishkuro yurishkuro added the changelog:test Change that's adding missing tests or correcting existing tests label Mar 21, 2024
@yurishkuro
Copy link
Member

@Pushkarm029 I doubt you can compare binary files like that, @james-ryans might have completely different binary representation because of timestamps. You can query db via jaeger-query/apiv3 JSON interface

@Pushkarm029
Copy link
Member Author

Pushkarm029 commented Mar 21, 2024

image
traces are being stored in the database, so I think there may be some problem with the data receiver.

@yurishkuro
Copy link
Member

traces are being stored in the database, so I think there may be some problem with the data receiver.

perhaps we can add logging to the data receiver that shows which query it is executing against the backend. Then try to run the same query manually - if it returns the data it means you likely have some connectivity issue rather than data issue.

@james-ryans
Copy link
Contributor

perhaps we can add logging to the data receiver that shows which query it is executing against the backend.

Yes, I'm trying to help figure out how to add logging similar to the OTEL collector's components log to the data receiver.

I just finished debugging the issue, it was sourced from the artificial jaeger receiver. In Badger, the spanReader.FindTraces query params require StartTimeMin and StartTimeMax which causes the error below. This might also happen to @akagami-harsh's PR at #5287.

Failed to consume traces from consumer	{"error": "stream error: rpc error: code = Unknown desc = start and end time must be set"}

I'll create a new PR to fix this issue and also add logging to the data receiver. Sorry for the inconvenience.

@Pushkarm029
Copy link
Member Author

Pushkarm029 commented Mar 21, 2024

Sorry for the inconvenience.

Don't feel sorry; after all, we all are here to learn❤️

Signed-off-by: Pushkar Mishra <[email protected]>
@Pushkarm029 Pushkarm029 marked this pull request as ready for review March 21, 2024 19:32
@Pushkarm029 Pushkarm029 requested a review from a team as a code owner March 21, 2024 19:32
yurishkuro pushed a commit that referenced this pull request Mar 23, 2024
… function. (#5288)

## Which problem is this PR solving?
- Fixes #5281 issue for not being able to consume traces

## Description of the changes
- Added `StartTimeMin` and `StartTimeMax` query params with 1h lookback
to `consumeTraces` function.
- Added logging to the data receivers with the OTelCol codes below as
the references.
-
https://github.com/open-telemetry/opentelemetry-collector/blob/main/otelcol/unmarshaler.go#L45
-
https://github.com/open-telemetry/opentelemetry-collector/blob/main/service/telemetry/telemetry.go#L118

## How was this change tested?
- Tested manually.

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: James Ryans <[email protected]>
@Pushkarm029
Copy link
Member Author

Why closed?

@yurishkuro
Copy link
Member

Auto-closed because the other PR had the text Fixes #5281 issue, need to be careful with those.

@yurishkuro yurishkuro reopened this Mar 24, 2024
Signed-off-by: Pushkar Mishra <[email protected]>
@yurishkuro
Copy link
Member

@Pushkarm029 do we still need this PR or is it covered by #5355 ?

@Pushkarm029
Copy link
Member Author

Nope, I'm closing it now.

@Pushkarm029 Pushkarm029 closed this May 3, 2024
@Pushkarm029 Pushkarm029 deleted the badger_integration branch May 3, 2024 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:test Change that's adding missing tests or correcting existing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants