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

Track JS errors only once per page view #17288

Merged
merged 20 commits into from Mar 28, 2021
Merged

Conversation

flamisz
Copy link
Contributor

@flamisz flamisz commented Mar 2, 2021

Description:

fixes #17163

Review

  • Functional review done
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@flamisz flamisz self-assigned this Mar 2, 2021
@flamisz flamisz added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Mar 2, 2021
@flamisz flamisz changed the title Track JS errors only per page view Track JS errors only once per page view Mar 2, 2021
@flamisz
Copy link
Contributor Author

flamisz commented Mar 2, 2021

build js

@flamisz
Copy link
Contributor Author

flamisz commented Mar 2, 2021

build js

@flamisz flamisz added this to the 4.5.0 milestone Mar 9, 2021
@sgiehl
Copy link
Member

sgiehl commented Mar 9, 2021

build js

Copy link
Member

@sgiehl sgiehl 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 so far.
@tsteur Not sure if that is something we should mention in the changelog. Or maybe we should update https://matomo.org/faq/how-to/how-do-i-enable-basic-javascript-error-tracking-and-reporting-in-matomo-browser-console-error-messages/ after merging and mention, that each error will only be tracked once?

@sgiehl sgiehl removed the Needs Review PRs that need a code review label Mar 9, 2021
@flamisz
Copy link
Contributor Author

flamisz commented Mar 11, 2021

@tsteur I think it's a good idea to mention it somewhere, we only track once per page view. The original issue name will get into the changelog, won't it?

@tsteur
Copy link
Member

tsteur commented Mar 11, 2021

build js

@tsteur
Copy link
Member

tsteur commented Mar 11, 2021

@flamisz updated https://matomo.org/faq/how-to/how-do-i-enable-basic-javascript-error-tracking-and-reporting-in-matomo-browser-console-error-messages/ and could also add it to the developer changelog https://github.com/matomo-org/matomo/blob/4.x-dev/CHANGELOG.md . wouldn't need to mention the issue number

@tsteur tsteur modified the milestones: 4.5.0, 4.3.0 Mar 11, 2021
@flamisz
Copy link
Contributor Author

flamisz commented Mar 11, 2021

build js

@diosmosis
Copy link
Member

@tsteur @sgiehl is this something we'd want to write a quick test for in https://github.com/matomo-org/matomo/blob/4.x-dev/tests/javascript/index.php

@tsteur
Copy link
Member

tsteur commented Mar 14, 2021

Yes, it be great to have a quick test for it 👍

js/piwik.js Outdated Show resolved Hide resolved
@flamisz
Copy link
Contributor Author

flamisz commented Mar 15, 2021

build js

@sgiehl
Copy link
Member

sgiehl commented Mar 16, 2021

@flamisz seems the javascript tests are still failing

@flamisz
Copy link
Contributor Author

flamisz commented Mar 17, 2021

build js

@flamisz
Copy link
Contributor Author

flamisz commented Mar 17, 2021

Hi @sgiehl, I fixed the js tests, but now the Travis CI - Pull Request build failed. What could it be? Is there a way to rebuild it? I couldn't find any (obviously without pushing any changes).

@diosmosis
Copy link
Member

diosmosis commented Mar 17, 2021

@flamisz there should be a "restart build" button:
image

you may need to authorize your github account w/ travis if it's not there (it might be in settings after logging in).

EDIT: that said, we generally only need one build to pass.

@flamisz
Copy link
Contributor Author

flamisz commented Mar 28, 2021

build js

@flamisz
Copy link
Contributor Author

flamisz commented Mar 28, 2021

build js

@diosmosis diosmosis merged commit 8402837 into 4.x-dev Mar 28, 2021
@diosmosis diosmosis deleted the 17163-js-error-tracking branch March 28, 2021 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JS Error tracking should record the same error only once per page view
4 participants