[BCF-3178] - Improve err handling and logs for in memory data source cache #12907

ilija42 · 2024-04-21T17:28:56Z

Fixes updateCache() change/bug that only allowed pipeline results where none of the data sources failed.
Improves err handling, logs and err messages.

github-actions · 2024-04-21T17:29:13Z

I see you updated files related to core. Please run pnpm changeset in the root directory to add a changeset as well as in the text include at least one of the following tags:

#added For any new functionality added.
#breaking_change For any functionality that requires manual action for the node to boot.
#bugfix For bug fixes.
#changed For any change to the existing functionality.
#db_update For any feature that introduces updates to database schema.
#deprecation_notice For any upcoming deprecation functionality.
#internal For changesets that need to be excluded from the final changelog.
#nops For any feature that is NOP facing and needs to be in the official Release Notes for the release.
#removed For any functionality/config that is removed.
#updated For any functionality that is updated.
#wip For any change that is not ready yet and external communication about it should be held off till it is feature complete.

github-actions · 2024-04-21T17:30:44Z

I see you added a changeset file but it does not contain a tag. Please edit the text include at least one of the following tags:

#added For any new functionality added.
#breaking_change For any functionality that requires manual action for the node to boot.
#bugfix For bug fixes.
#changed For any change to the existing functionality.
#db_update For any feature that introduces updates to database schema.
#deprecation_notice For any upcoming deprecation functionality.
#internal For changesets that need to be excluded from the final changelog.
#nops For any feature that is NOP facing and needs to be in the official Release Notes for the release.
#removed For any functionality/config that is removed.
#updated For any functionality that is updated.
#wip For any change that is not ready yet and external communication about it should be held off till it is feature complete.

ilija42 · 2024-04-21T17:32:28Z

core/services/ocrcommon/data_source.go

@@ -294,31 +293,30 @@ func (ds *inMemoryDataSourceCache) updateCache(ctx context.Context) error {
 	ds.mu.Lock()
 	defer ds.mu.Unlock()

-	// check for any errors
-	_, latestTrrs, latestUpdateErr := ds.executeRun(ctx)


This checked for any errors in the pipeline results, which doesn't make sense since multiple bridge data sources are used and not always all of them are successful

ilija42 · 2024-04-21T17:33:32Z

core/services/ocrcommon/data_source.go

+	// update cache values
+	ds.latestTrrs = latestTrrs
+	ds.latestResult = ds.latestTrrs.FinalResult(ds.lggr)
+	ds.latestUpdateErr = nil


moved this down to be more cautious since we now don't check for all errors, although median task should handle everything properly

krehermann

this needs a test if we are going to claim it fixes a bug, don't you think?

cl-sonarqube-production · 2024-04-22T17:08:06Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
50.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

ilija42 · 2024-04-22T17:28:24Z

this needs a test if we are going to claim it fixes a bug, don't you think?

I just reverted changes to previous behaviour that already existed. I just didn't realise how pipeline propagates errors and tried to handle errors on my own even if TaskResults struct methods already did that.

Its hard to write a test that actually tests this. TLDR pipeline has its own err handling logic which is complex and I can't mock this behaviour, it would require a full integration test with this exact scenario.

krehermann · 2024-04-22T17:49:30Z

this needs a test if we are going to claim it fixes a bug, don't you think?

I just reverted changes to previous behaviour that already existed. I just didn't realise how pipeline propagates errors and tried to handle errors on my own even though TaskResults struct methods already do that.

Its hard to write a test that actually tests this. TLDR pipeline has its own err handling logic which is complex and I can't mock this behaviour, it would require a full integration test with this exact scenario.

I see. I couldn't tell it was a reversion. It reads like a new change.

…cache (#12907) * Fix inMemoryDataSourceCache get() warn log formatting * Fix in mem ds cache updateCache() to save runs even if some ds failed * Improve in memory data source cache Observe() logs and error messages * Add changeset

* Bump version and update CHANGELOG for core 2.11.0 Signed-off-by: Sneha Agnihotri <[email protected]> * Add jfpc cache staleness alert to config and pass it into jfpc cache (#12595) * Add jfpc cache staleness alert to config and pass it into jfpc cache * Extract JuelsPerFeeCoinCache cfg into a separate struct, improve naming * Changeset * Update config validation test * Reduce changeset to minor * [BCF-3178] - Improve err handling and logs for in memory data source cache (#12907) * Fix inMemoryDataSourceCache get() warn log formatting * Fix in mem ds cache updateCache() to save runs even if some ds failed * Improve in memory data source cache Observe() logs and error messages * Add changeset * Bumping solana validator (#12832) * Bumping solana * Bumping the checksum for solana validator * Update release changelog with cherry pick commits from develop Signed-off-by: Sneha Agnihotri <[email protected]> * Cherry pick: enable jpfc cache by default (#12973) * Draft * Check JuelsPerFeeCoinCache before checking Disable param * Add test * Update NewInMemoryDataSourceCache * Update NewInMemoryDataSourceCache * Update release changelog with cherry pick commits from develop Signed-off-by: Sneha Agnihotri <[email protected]> * Remove duplicates Signed-off-by: Sneha Agnihotri <[email protected]> * Finalize date on changelog for 2.11.0 Signed-off-by: Sneha Agnihotri <[email protected]> * Remove codecov (#13020) (cherry picked from commit 2e99468) --------- Signed-off-by: Sneha Agnihotri <[email protected]> Co-authored-by: ilija42 <[email protected]> Co-authored-by: Damjan Smickovski <[email protected]> Co-authored-by: george-dorin <[email protected]> Co-authored-by: HenryNguyen5 <[email protected]>

ilija42 added 3 commits April 21, 2024 15:13

Fix inMemoryDataSourceCache get() warn log formatting

86fd239

Fix in mem ds cache updateCache() to save runs even if some ds failed

5bcf9cd

Improve in memory data source cache Observe() logs and error messages

4ab5ed4

ilija42 requested review from a team as code owners April 21, 2024 17:28

ilija42 temporarily deployed to sdlc April 21, 2024 17:29 — with GitHub Actions Inactive

ilija42 temporarily deployed to sdlc April 21, 2024 17:30 — with GitHub Actions Inactive

ilija42 commented Apr 21, 2024

View reviewed changes

ilija42 changed the title ~~Improve err handling and logs for in memory data source cache~~ BCF-3178 - Improve err handling and logs for in memory data source cache Apr 21, 2024

ilija42 force-pushed the fix-inmemdscache-err-handling branch from fbd6f37 to 87331bb Compare April 21, 2024 19:03

ilija42 temporarily deployed to sdlc April 21, 2024 19:03 — with GitHub Actions Inactive

ilija42 changed the title ~~BCF-3178 - Improve err handling and logs for in memory data source cache~~ [BCF-3178] - Improve err handling and logs for in memory data source cache Apr 21, 2024

ilija42 requested a review from jmank88 April 22, 2024 09:43

krehermann reviewed Apr 22, 2024

View reviewed changes

Add changeset

23ef713

ilija42 force-pushed the fix-inmemdscache-err-handling branch from 87331bb to 23ef713 Compare April 22, 2024 16:52

ilija42 temporarily deployed to sdlc April 22, 2024 16:52 — with GitHub Actions Inactive

krehermann approved these changes Apr 22, 2024

View reviewed changes

ilija42 added this pull request to the merge queue Apr 22, 2024

Merged via the queue into develop with commit f0439ec Apr 22, 2024
105 checks passed

ilija42 deleted the fix-inmemdscache-err-handling branch April 22, 2024 18:06

app-token-issuer-infra-releng bot mentioned this pull request Apr 24, 2024

[DO NOT MERGE] Release Preview - Changeset #12949

Closed

snehaagni mentioned this pull request May 1, 2024

chore/release 2.11.0 to develop #13067

Merged

samsondav mentioned this pull request May 17, 2024

release/2.11.1 hotfix panic #13244

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BCF-3178] - Improve err handling and logs for in memory data source cache #12907

[BCF-3178] - Improve err handling and logs for in memory data source cache #12907

ilija42 commented Apr 21, 2024 •

edited

Loading

github-actions bot commented Apr 21, 2024

github-actions bot commented Apr 21, 2024

ilija42 Apr 21, 2024

ilija42 Apr 21, 2024 •

edited

Loading

krehermann left a comment

cl-sonarqube-production bot commented Apr 22, 2024

ilija42 commented Apr 22, 2024 •

edited

Loading

krehermann commented Apr 22, 2024 •

edited by ilija42

Loading

[BCF-3178] - Improve err handling and logs for in memory data source cache #12907

[BCF-3178] - Improve err handling and logs for in memory data source cache #12907

Conversation

ilija42 commented Apr 21, 2024 • edited Loading

github-actions bot commented Apr 21, 2024

github-actions bot commented Apr 21, 2024

ilija42 Apr 21, 2024

Choose a reason for hiding this comment

ilija42 Apr 21, 2024 • edited Loading

Choose a reason for hiding this comment

krehermann left a comment

Choose a reason for hiding this comment

cl-sonarqube-production bot commented Apr 22, 2024

Quality Gate passed

ilija42 commented Apr 22, 2024 • edited Loading

krehermann commented Apr 22, 2024 • edited by ilija42 Loading

ilija42 commented Apr 21, 2024 •

edited

Loading

ilija42 Apr 21, 2024 •

edited

Loading

ilija42 commented Apr 22, 2024 •

edited

Loading

krehermann commented Apr 22, 2024 •

edited by ilija42

Loading