Skip to content

feat(rum-core): use etag for fetching config#439

Merged
vigneshshanmugam merged 3 commits intoelastic:masterfrom
hmdhk:central-config-etag
Oct 23, 2019
Merged

feat(rum-core): use etag for fetching config#439
vigneshshanmugam merged 3 commits intoelastic:masterfrom
hmdhk:central-config-etag

Conversation

@hmdhk
Copy link
Copy Markdown
Contributor

@hmdhk hmdhk commented Sep 26, 2019 •
edited by vigneshshanmugam
Loading

Part of #253
store remote config in localStorage

@hmdhk hmdhk force-pushed the central-config-etag branch from 9e0646c to 064dd7f Compare September 26, 2019 10:57
@codecov-io
Copy link
Copy Markdown

codecov-io commented Sep 29, 2019 •
edited
Loading

Codecov Report

Merging #439 into master will decrease coverage by 0.28%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master     #439      +/-   ##
==========================================
- Coverage   94.04%   93.75%   -0.29%     
==========================================
  Files          40       40              
  Lines        2081     1971     -110     
  Branches      435      396      -39     
==========================================
- Hits         1957     1848     -109     
+ Misses        121      120       -1     
  Partials        3        3
Impacted Files Coverage Δ
...c/performance-monitoring/performance-monitoring.js 93.9% <ø> (-0.58%) ⬇️
packages/rum-core/src/common/utils.js 95.93% <100%> (+0.55%) ⬆️
...rum-core/src/performance-monitoring/transaction.js 98.66% <100%> (+1.06%) ⬆️
...s/rum-core/src/performance-monitoring/span-base.js 93.18% <100%> (ø) ⬆️
packages/rum-core/src/common/apm-server.js 86.97% <100%> (-1.14%) ⬇️
...ckages/rum-core/src/error-logging/error-logging.js 98.61% <100%> (-0.04%) ⬇️
...s/rum-core/src/performance-monitoring/breakdown.js 95.65% <91.66%> (-2.23%) ⬇️
.../src/performance-monitoring/transaction-service.js 91.72% <92%> (-0.59%) ⬇️
... and 4 more

vigneshshanmugam
vigneshshanmugam reviewed Oct 9, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

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

Thanks @jahtalab, The approach looks good to me except for the localStorage part. It would be better if we use SessionStorage instead of LocalStorage since it has less expiry than LocalStorage which is cleared only on explicit browser cache.

Added few comments on the code for other parts.

@hmdhk
Copy link
Copy Markdown
Contributor Author

hmdhk commented Oct 10, 2019

Thanks for the review @vigneshshanmugam ,

Re. sessionStorage, I could see benefits with taking both approaches, e.g using localStorage would prevent the page from fetching the configuration when the application is opened in multiple tabs or opened later in the same device.

But I also like sessionStorage since we won't store any permanent data on the users' machine.

I will change it for now, we can reconsider in the future if needed!

@jalvz jalvz mentioned this pull request Oct 11, 2019
Merged
hmdhk added 2 commits October 11, 2019 14:14
@hmdhk
feat(rum-core): use etag for fetching config
e2da762 
store remote config in localStorage
@hmdhk
use sessionStorage
8df861f 
add more tests
@hmdhk hmdhk force-pushed the central-config-etag branch from 064dd7f to 8df861f Compare October 14, 2019 13:28
vigneshshanmugam
vigneshshanmugam reviewed Oct 15, 2019
View reviewed changes
vigneshshanmugam
vigneshshanmugam reviewed Oct 15, 2019
View reviewed changes
vigneshshanmugam
vigneshshanmugam approved these changes Oct 23, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

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

Thanks @jahtalab Looks good.

@vigneshshanmugam vigneshshanmugam merged commit bac0e15 into elastic:master Oct 23, 2019
David-Development pushed a commit to David-Development/apm-agent-rum-js that referenced this pull request Oct 20, 2021
@jahtalab @vigneshshanmugam
feat(rum-core): use etag for fetching config (elastic#439)
656197e 
* feat(rum-core): use etag for fetching config

store remote config in localStorage

* use sessionStorage

add more tests

* ignore falsy values in setLocalConfig
devcorpio pushed a commit to bmorelli25/apm-agent-rum-js that referenced this pull request Jan 25, 2022
@jahtalab @devcorpio
feat(rum-core): use etag for fetching config (elastic#439)
2b4b38d 
* feat(rum-core): use etag for fetching config

store remote config in localStorage

* use sessionStorage

add more tests

* ignore falsy values in setLocalConfig
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vigneshshanmugam vigneshshanmugam vigneshshanmugam approved these changes

+1 more reviewer

@jalvz jalvz jalvz left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hmdhk @codecov-io @jalvz @vigneshshanmugam