Entity Service and Config Service Configurable Timeouts #92

suddendust · 2021-07-11T06:30:07Z

Description

Like the PR here, this change is to make client timeout for the Attribute Service configurable.

Testing

Deployed and tested the application locally. Scenarios:

Missing config: Default timeout is used.
Timeout configured: Configured value is used.

Checklist:

My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
Any dependent changes have been merged and published in downstream modules

codecov · 2021-07-11T06:31:11Z

Codecov Report

Merging #92 (d009bf9) into main (2cdb158) will decrease coverage by 0.06%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##               main      #92      +/-   ##
============================================
- Coverage     22.26%   22.19%   -0.07%     
  Complexity       66       66              
============================================
  Files            64       64              
  Lines          1590     1595       +5     
  Branches         49       49              
============================================
  Hits            354      354              
- Misses         1229     1234       +5     
  Partials          7        7

Flag	Coverage Δ
unit	`22.19% <0.00%> (-0.07%)`	⬇️

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

Impacted Files	Coverage Δ
...aphql/spaces/dao/ConfigServiceSpacesConfigDao.java	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cdb158...d009bf9. Read the comment docs.

suddendust · 2021-07-11T14:31:19Z

@aaron-steinfeld Same changes what we did for Gateway service. Please review once you're back. Thanks :)

kotharironak · 2021-07-12T07:34:16Z

@suddendust Can you take care of snyk failure? If, not upgrade, can you update the .snyk file by extending the date?

kotharironak · 2021-07-12T07:56:55Z

...raphql-service/src/main/java/org/hypertrace/graphql/service/DefaultGraphQlServiceConfig.java

@@ -22,16 +24,19 @@

  private static final String ATTRIBUTE_SERVICE_HOST_PROPERTY = "attribute.service.host";
  private static final String ATTRIBUTE_SERVICE_PORT_PROPERTY = "attribute.service.port";
+  private static final String ATTRIBUTE_SERVICE_PORT_TIMEOUT = "attribute.service.timeout";


why is this ATTRIBUTE_SERVICE_PORT_TIMEOUT instead of ATTRIBUTE_SERVICE_CLIENT_TIMEOUT?

Typo, addressed.

kotharironak · 2021-07-12T07:57:29Z

...raphql-service/src/main/java/org/hypertrace/graphql/service/DefaultGraphQlServiceConfig.java

+    this.configServiceTimeout =
+        getSuppliedDurationOrFallback(
+            () -> untypedConfig.getDuration(CONFIG_SERVICE_CLIENT_TIMEOUT));
+    this.entityServiceTimeout =


Is this use anywhere?

entityServiceTimeout is not being used anywhere. EntityTypeCachingClient uses a hardcoded value of 10s here. Should this value be replaced with this timeout?

I was checking at the GQL level, if it is not used, should we add it or should we add it when it is used?

Yes that's a fair point. I just added it so that we have a consistent configuration (as all other services had a timeout). Should I remove it (we do have a default fallback value though so even if you don't give the config it'll work).

I think we can keep it for consistency as we are going with the different services that might have a different timeout requirement, and already have fields for (host, port) for this one.

aaron-steinfeld · 2021-07-12T11:38:17Z

...raphql-service/src/main/java/org/hypertrace/graphql/service/DefaultGraphQlServiceConfig.java

+            () -> untypedConfig.getDuration(ENTITY_SERVICE_CLIENT_TIMEOUT));
+  }
+
+  private Duration getSuppliedDurationOrFallback(Supplier<Duration> durationSupplier) {


same nit about using optionallyGet

Addressed. Thanks :)

aaron-steinfeld

Did you want to pull in the changes to the core submodule in this PR?

suddendust · 2021-07-12T14:02:20Z

Did you want to pull in the changes to the core submodule in this PR?

Just did @aaron-steinfeld

aaron-steinfeld

Thanks @suddendust !

github-actions · 2021-07-12T14:11:20Z

Unit Test Results

10 files ±0 10 suites ±0 15s ⏱️ ±0s
20 tests ±0 20 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 510a988. ± Comparison against base commit 2cdb158.

* refactor: attribute association expression rename * fix: vulnerable dependency

Entity service and config service configurable timeouts

502e8c7

suddendust requested a review from a team as a code owner July 11, 2021 06:30

suddendust mentioned this pull request Jul 11, 2021

Default values for Entity and Config service timeouts in application.… hypertrace/hypertrace-service#100

Open

3 tasks

.synk update expiry date

f39ddd9

kotharironak reviewed Jul 12, 2021

View reviewed changes

Fix typo

22284e6

aaron-steinfeld previously approved these changes Jul 12, 2021

View reviewed changes

Reuse optionallyGet in the implementation of getTimeoutOrFallback

86bbfe5

suddendust dismissed aaron-steinfeld’s stale review via 86bbfe5 July 12, 2021 11:50

aaron-steinfeld previously approved these changes Jul 12, 2021

View reviewed changes

chore: submodule ref update

d009bf9

suddendust dismissed aaron-steinfeld’s stale review via d009bf9 July 12, 2021 14:01

aaron-steinfeld approved these changes Jul 12, 2021

View reviewed changes

aaron-steinfeld merged commit 510a988 into hypertrace:main Jul 12, 2021

skjindal93 pushed a commit that referenced this pull request Jun 21, 2024

refactor: attribute association expression rename (#92)

43f084b

* refactor: attribute association expression rename * fix: vulnerable dependency

Entity Service and Config Service Configurable Timeouts #92

Entity Service and Config Service Configurable Timeouts #92

Uh oh!

Conversation

suddendust commented Jul 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Checklist:

Uh oh!

codecov bot commented Jul 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

suddendust commented Jul 11, 2021

Uh oh!

kotharironak commented Jul 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaron-steinfeld left a comment

Choose a reason for hiding this comment

Uh oh!

suddendust commented Jul 12, 2021

Uh oh!

aaron-steinfeld left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 12, 2021

Unit Test Results

Uh oh!

Uh oh!

suddendust commented Jul 11, 2021 •

edited

Loading

codecov bot commented Jul 11, 2021 •

edited

Loading