Skip to content
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

fix: Allow title override in page attributes #508

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

qhanam
Copy link
Member

@qhanam qhanam commented Feb 9, 2024

For every page view, the web client updates the title attribute with the value in window.document.title. This means that the title attribute still cannot be overridden (i.e., #430 did not actually solve #493).

This change allows the title attribute to be overridden when recordPageView is used.

Before

document.title = "sensitive";
pageManager.recordPageView({
            pageId: '/home',
            pageAttributes: {
                title: 'override'
            }
        });
// attributes: { pageId: '/home', title: 'sensitive' }

After

document.title = "sensitive";
pageManager.recordPageView({
            pageId: '/home',
            pageAttributes: {
                title: 'override'
            }
        });
// attributes: { pageId: '/home', title: 'override' }

Discussion

Options for fixing this include the following.

  1. Stop recording the page title.
  2. Record title at the start of the session only.
  3. Session attributes cannot be overwritten.
  4. Allow page attributes to be overwritten.

All of these options have problems.

  • Regarding (1), we stop giving users a common piece of information.
  • Regarding (2), if the title changes during route changes, the new title will not be recorded.
  • Regarding (3), this would create special classes of attributes.
  • Regarding (4), requires the application to explicitly recording page views, and overwrite the title for each page view.

I am recommending option (4), because it has the least risk with respect to backwards compatibility.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@qhanam qhanam requested review from williazz and ps863 February 9, 2024 21:50
@williazz
Copy link
Contributor

williazz commented Feb 9, 2024

@qhanam
Copy link
Member Author

qhanam commented Feb 9, 2024

can the override logic be implemented by swapping these two lines instead?

The only other attributes that could be overridden are pageId and pageTags. These are passed as arguments to recordPageView along with the custom attributes, and so in the original spec we decided that the arguments would take precedence over the attributes.

@qhanam qhanam merged commit a6a1ad8 into aws-observability:main Feb 12, 2024
3 checks passed
@qhanam qhanam deleted the fix/page-attribute-override branch February 12, 2024 17:23
limhjgrace pushed a commit to limhjgrace/aws-rum-web that referenced this pull request Feb 26, 2024
limhjgrace pushed a commit to limhjgrace/aws-rum-web that referenced this pull request Feb 26, 2024
limhjgrace added a commit that referenced this pull request Feb 26, 2024
* fix: Allow title override in page attributes (#508)

* chore(release): 1.17.1

---------

Co-authored-by: Quinn Hanam <qhanam@gmail.com>
williazz added a commit that referenced this pull request Apr 9, 2024
* fix: Allow title override in page attributes (#508)

* chore(deps-dev): bump ip from 1.1.5 to 1.1.9 (#513)

Bumps [ip](https://github.com/indutny/node-ip) from 1.1.5 to 1.1.9.
- [Commits](indutny/node-ip@v1.1.5...v1.1.9)

---
updated-dependencies:
- dependency-name: ip
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore(deps-dev): bump follow-redirects from 1.15.4 to 1.15.6 (#525)

Bumps [follow-redirects](https://github.com/follow-redirects/follow-redirects) from 1.15.4 to 1.15.6.
- [Release notes](https://github.com/follow-redirects/follow-redirects/releases)
- [Commits](follow-redirects/follow-redirects@v1.15.4...v1.15.6)

---
updated-dependencies:
- dependency-name: follow-redirects
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore(deps-dev): bump webpack-dev-middleware from 6.0.1 to 6.1.2 (#528)

Bumps [webpack-dev-middleware](https://github.com/webpack/webpack-dev-middleware) from 6.0.1 to 6.1.2.
- [Release notes](https://github.com/webpack/webpack-dev-middleware/releases)
- [Changelog](https://github.com/webpack/webpack-dev-middleware/blob/v6.1.2/CHANGELOG.md)
- [Commits](webpack/webpack-dev-middleware@v6.0.1...v6.1.2)

---
updated-dependencies:
- dependency-name: webpack-dev-middleware
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* feat: retry with exponential backoff (#501)

* feat: retry with exponential backoff

* fix

* test

* feat: limit retries to 5xx and 429 (#500)

* feat: limit PutRumEvents retry to 5xx

* feat: add 429

* cleanup

* cleanup

* docs

* doc

* fix: fmt

* chore(deps-dev): bump express from 4.18.1 to 4.19.2 (#531)

Bumps [express](https://github.com/expressjs/express) from 4.18.1 to 4.19.2.
- [Release notes](https://github.com/expressjs/express/releases)
- [Changelog](https://github.com/expressjs/express/blob/master/History.md)
- [Commits](expressjs/express@4.18.1...4.19.2)

---
updated-dependencies:
- dependency-name: express
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* feat: keep alive when dispatch fails (#524)

* feat: disableOnFail

* doc: add config doc

* Revert "doc: add config doc"

This reverts commit ec1f74f.

* Revert "feat: disableOnFail"

This reverts commit 67ed7b3.

* feat: only disable RUM when dispatch fails with 403 or 404

* chore: add unit tests

* feat: add 401

* fix: record resources with invalid names (#532)

* fix: Ignore URL construction error from invalid performance resource event

* fix: Throw error when URL construction fails for invalid performance resource event

* fix: Ignore error thrown from URL construction

* test: add unit test

* fix: record resources with invalid names

* fix: Update unit test for invalid url

* fix: Update hostname typo in isPutRumEventsCall tests

---------

Co-authored-by: Billy <billyzh@amazon.com>

* chore(release): 1.17.2

* Revert "Sync changes from main for 1.17.2 patch release"

* fix: record resources with invalid names (#532)

* fix: Ignore URL construction error from invalid performance resource event

* fix: Throw error when URL construction fails for invalid performance resource event

* fix: Ignore error thrown from URL construction

* test: add unit test

* fix: record resources with invalid names

* fix: Update unit test for invalid url

* fix: Update hostname typo in isPutRumEventsCall tests

---------

Co-authored-by: Billy <billyzh@amazon.com>

* Update changelog

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Quinn Hanam <qhanam@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Billy <billyzh@amazon.com>
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.

3 participants