Skip to content

[backend] Added bulk create expectation traces endpoint #2945

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

Open
wants to merge 4 commits into
base: release/current
Choose a base branch
from

Conversation

impolitepanda
Copy link
Member

@impolitepanda impolitepanda commented Apr 14, 2025

Proposed changes

  • Added bulk create expectation traces endpoint

Related issues

Checklist

  • I consider the submitted work as finished
  • I tested the code for its functionality
  • I wrote test cases for the relevant uses case
  • I added/update the relevant documentation (either on github or on notion)
  • Where necessary I refactored code to improve the overall quality
  • For bug fix -> I implemented a test that covers the bug

Copy link

codecov bot commented Apr 14, 2025

Codecov Report

Attention: Patch coverage is 90.14085% with 7 lines in your changes missing coverage. Please review.

Project coverage is 39.98%. Comparing base (3b9c91c) to head (8e288e2).
Report is 56 commits behind head on release/current.

Files with missing lines Patch % Lines
...a/io/openbas/service/InjectExpectationService.java 84.84% 3 Missing and 2 partials ⚠️
...t_expectation_trace/InjectExpectationTraceApi.java 81.81% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@                  Coverage Diff                  @@
##             release/current    #2945      +/-   ##
=====================================================
- Coverage              40.08%   39.98%   -0.11%     
- Complexity              2123     2173      +50     
=====================================================
  Files                    646      652       +6     
  Lines                  20024    20686     +662     
  Branches                1368     1493     +125     
=====================================================
+ Hits                    8027     8271     +244     
- Misses                 11556    11956     +400     
- Partials                 441      459      +18     

☔ 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.

@impolitepanda impolitepanda marked this pull request as ready for review April 15, 2025 02:18
@antoinemzs antoinemzs self-requested a review April 15, 2025 07:25
Comment on lines +105 to +112
// Dedupe from DB
List<SimpleRawExpectationTrace> rawsfromDB1 =
this.injectExpectationTraceRepository.findAllTracesNewerThan(
oldestAlertDate.get().truncatedTo(ChronoUnit.SECONDS));
Set<SimpleRawExpectationTrace> fromDB = new HashSet<>(rawsfromDB1);

// Removing duplicate traces
traces.keySet().removeAll(fromDB);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you not better dedupe using the alert link which bears the alert id from the input object, and avoid all the timestamp black magic?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd love to, but I don't think it's doable with correct performance. We can receive 10K+ inputs. Unless I'm missing something, what you suggest would require to either select with a 10K+ IN clause with alert links as a criteria, or make 10K+ different select calls to the DB.
Could you elaborate on your proposition?

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't think of something better than a custom insert query with the ON CONFLICT DO NOTHING clause from postgres https://www.postgresql.org/docs/current/sql-insert.html#SQL-ON-CONFLICT

And the conflict would be on the alert_link column.

The JpaRepository does not expose this natively as it's a postgres feature (not standard SQL).

Here are examples as native queries:

Maybe worth a thought, especially if we're talking about five orders of magnitude?

The caveat here is that it would not account for alerts missing from subsequent batches (which would be deleted in the current code, not with the design discussed here).

@impolitepanda impolitepanda changed the title Added bulk create expectation traces endpoint [backend] Added bulk create expectation traces endpoint Apr 18, 2025
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.

2 participants