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

Send log messages to the constellation #11841

Merged
merged 1 commit into from Jul 15, 2016

Conversation

@asajeffrey
Copy link
Member

asajeffrey commented Jun 23, 2016

Send all warnings and errors to the constellation. Warnings are bufferred up, and included in any subsequent error reports. Errors are reported in the same way as panics.

Note that this can't merge yet, as it needs rust-lang/log#86 to land.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #11776 (github issue number if applicable).
  • These changes do not require tests because we don't test crash reporting.

This change is Reviewable

@highfive
Copy link

highfive commented Jun 23, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script_traits/Cargo.toml, components/script_traits/Cargo.toml, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script_traits/lib.rs, components/script_traits/lib.rs
@asajeffrey asajeffrey force-pushed the asajeffrey:constellation-logging branch from 1673b5f to cdcc1e8 Jun 23, 2016
@asajeffrey
Copy link
Member Author

asajeffrey commented Jun 23, 2016

Rebased.

@bors-servo
Copy link
Contributor

bors-servo commented Jul 3, 2016

The latest upstream changes (presumably #12178) made this pull request unmergeable. Please resolve the merge conflicts.

@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 11, 2016

rust-lang/log#86 has landed, it just needs to make its way to crates.io.

@asajeffrey asajeffrey force-pushed the asajeffrey:constellation-logging branch from cdcc1e8 to ad862c0 Jul 11, 2016
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 11, 2016

Rebased.

@jdm jdm added the S-fails-travis label Jul 11, 2016
@asajeffrey asajeffrey force-pushed the asajeffrey:constellation-logging branch from ad862c0 to 0ce4055 Jul 11, 2016
@asajeffrey asajeffrey changed the title Sent log messages to the constellation [DO NOT MERGE YET] Send log messages to the constellation Jul 11, 2016
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 11, 2016

env_logger 0.3.4 is now out, so this can merge. r? @Ms2ger

asajeffrey added a commit to asajeffrey/servo that referenced this pull request Jul 14, 2016
…by integrated logging and issue reporting.
asajeffrey added a commit to asajeffrey/servo that referenced this pull request Jul 14, 2016
…by integrated logging and issue reporting.
@@ -304,6 +312,97 @@ enum ExitPipelineMode {
Force,
}

/// A logger directed at the constellation from content processes
#[derive(Clone)]
pub struct FromScriptLogger {

This comment has been minimized.

@Manishearth

Manishearth Jul 15, 2016

Member

We could probably have a FromLogger<T> struct with a common Log impl, where T is FromFooMsg, and implements a trait which provides a get_msg(Option<PipelineId>, Option<String>, LogEntry) -> Self method

@Manishearth
Copy link
Member

Manishearth commented Jul 15, 2016

r=me with or without that trait abstraction

@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 15, 2016

I think since there's only two instances, the extra level of generality won't buy us that much. Thanks for looking the code over!

@bors-servo r=Manishearth

@bors-servo
Copy link
Contributor

bors-servo commented Jul 15, 2016

📌 Commit bc1dd50 has been approved by Manishearth

@bors-servo
Copy link
Contributor

bors-servo commented Jul 15, 2016

Testing commit bc1dd50 with merge d97d297...

bors-servo added a commit that referenced this pull request Jul 15, 2016
Send log messages to the constellation

<!-- Please describe your changes on the following line: -->

Send all warnings and errors to the constellation. Warnings are bufferred up, and included in any subsequent error reports. Errors are reported in the same way as panics.

Note that this can't merge yet, as it needs rust-lang/log#86 to land.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #11776 (github issue number if applicable).
- [X] These changes do not require tests because we don't test crash reporting.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11841)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 15, 2016

💔 Test failed - linux-rel

@highfive
Copy link

highfive commented Jul 15, 2016

  ▶ Unexpected subtest result in /workers/WorkerGlobalScope_close.htm:
  └ PASS [expected FAIL]  WorkerGlobalScope close(): clear events queue 
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 15, 2016

OK, that's weird, did I accidentally fix a bug?

@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 15, 2016

IRC conversation: http://logs.glob.uno/?c=mozilla%23servo&s=15+Jul+2016&e=15+Jul+2016#c480756

TL;DR: when logs are reported to the constellation, we can check to see if the thread has panicked, and report Errors as Panics. The issue reporting and hard-failing mechanism can then just be used for panic, in particular hard-fail won't be triggered by JS errors.

@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 15, 2016

@Manishearth care to look over that last commit?

@asajeffrey asajeffrey force-pushed the asajeffrey:constellation-logging branch from eccd395 to 4442274 Jul 15, 2016
@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 15, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jul 15, 2016

📌 Commit 4442274 has been approved by Manishearth

@bors-servo
Copy link
Contributor

bors-servo commented Jul 15, 2016

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #12441
@bors-servo
Copy link
Contributor

bors-servo commented Jul 15, 2016

📌 Commit 4442274 has been approved by Manishearth

@asajeffrey
Copy link
Member Author

asajeffrey commented Jul 15, 2016

@bors-servo retry

  • CI oddness
@bors-servo
Copy link
Contributor

bors-servo commented Jul 15, 2016

Testing commit 4442274 with merge a5cd4b9...

bors-servo added a commit that referenced this pull request Jul 15, 2016
Send log messages to the constellation

<!-- Please describe your changes on the following line: -->

Send all warnings and errors to the constellation. Warnings are bufferred up, and included in any subsequent error reports. Errors are reported in the same way as panics.

Note that this can't merge yet, as it needs rust-lang/log#86 to land.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #11776 (github issue number if applicable).
- [X] These changes do not require tests because we don't test crash reporting.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11841)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 15, 2016

@bors-servo bors-servo merged commit 4442274 into servo:master Jul 15, 2016
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
homu Test successful
Details
asajeffrey added a commit to asajeffrey/servo that referenced this pull request Jul 15, 2016
…by integrated logging and issue reporting.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.