Skip to content

Gateway-Service Client Timeout Config #91

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

Merged

Conversation

suddendust
Copy link
Contributor

@suddendust suddendust commented Jul 9, 2021

Description

These changes are in context of: hypertrace/hypertrace-core-graphql#69. Currently, we hardcode the deadline as 10s. Now, GATEWAY_SERVICE_DEADLINE is read from application.conf

Testing

Deployed and tested the application manually. I am still seeing if I can write a UT for check if config value is being loaded properly.

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

This PR is dependent on: hypertrace/hypertrace-service#99

@suddendust suddendust requested a review from a team as a code owner July 9, 2021 16:58
@suddendust suddendust changed the title Gateway client deadline config Gateway-Service Client Timeout Config Jul 9, 2021
Copy link
Contributor

@aaron-steinfeld aaron-steinfeld 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 here, just need to update submodule to the now-merged squash commit 2b889a0071a28c481386eac7d6f506464c3bd2c4

@codecov
Copy link

codecov bot commented Jul 9, 2021

Codecov Report

Merging #91 (ea26c18) into main (cef8ab9) will decrease coverage by 0.08%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main      #91      +/-   ##
============================================
- Coverage     22.34%   22.26%   -0.09%     
  Complexity       66       66              
============================================
  Files            64       64              
  Lines          1584     1590       +6     
  Branches         49       49              
============================================
  Hits            354      354              
- Misses         1223     1229       +6     
  Partials          7        7              
Flag Coverage Δ
unit 22.26% <0.00%> (-0.09%) ⬇️

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

Impacted Files Coverage Δ
...ertrace/graphql/entity/dao/GatewayBaselineDao.java 0.00% <0.00%> (ø)
...ce/graphql/entity/dao/GatewayServiceEntityDao.java 0.00% <0.00%> (ø)
...raphql/explorer/dao/GatewayServiceExplorerDao.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 cef8ab9...ea26c18. Read the comment docs.

@suddendust
Copy link
Contributor Author

looks good here, just need to update submodule to the now-merged squash commit 2b889a0071a28c481386eac7d6f506464c3bd2c4

Done

@suddendust
Copy link
Contributor Author

suddendust commented Jul 10, 2021

The coverage seems to have gone down as these three classes have 0% coverage and this PR increases the total instructions. Should UTs for these three classes be added in this PR @aaron-steinfeld?

@aaron-steinfeld
Copy link
Contributor

The coverage seems to have gone down as these three classes have 0% coverage and this PR increases the total instructions. Should UTs for these three classes be added in this PR @aaron-steinfeld?

Not necessary for these changes - testing config read is generally more trouble than it's worth if we're working with primitives. The tests start duplicating the config and then you end up testing your test's config, rather than the one you'll use at runtime. Exception of course when we're doing something fancier with config (building data structures, class loading, deserializing objects etc).

On the DAO classes, we structure them such that they don't really have any logic and the compiler can more or less enforce correctness. So if you'd like to add tests for them, I'll happily approve, but I think there's much higher value classes to target.

@aaron-steinfeld aaron-steinfeld merged commit 2cdb158 into hypertrace:main Jul 10, 2021
@github-actions
Copy link

Unit Test Results

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

Results for commit 2cdb158. ± Comparison against base commit cef8ab9.

skjindal93 pushed a commit that referenced this pull request Jun 21, 2024
* feat: add support for attribute expressions

* feat: expression support in filter and sorting

* refactor: move stuff around

* refactor: more refactoring

* chore: temp for testing integration

* chore: more cleanup

* chore: clenaup dependencies, fix tests
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