Paths

Table of Contentst

-
src/applications/herald/engine/
-
applications/
-
herald/
-
engine/
-
HeraldEngine.php

Differential D25735

Log Herald rules with invalid keys via phlog()
ClosedPublic
Actions

Authored by aklapper on Jul 19 2024, 13:29.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15885: Herald rule referring to an invalid key silently fails

Commits

rPf6302c7ee300: Log Herald rules with invalid keys via phlog()

Summary

When Herald rules fail unexpectedly due to relying on a (now) invalid key, allow Phorge administrators to get aware by creating an entry in the error log via phlog() instead of crossing fingers that someone may from time to time get the idea to open and read Herald transcripts.

Refs T15885

Test Plan

See steps in T15885; check error log.

Diff Detail

Repository

rP Phorge

Branch

heraldRuleInvalid

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 1442
Build 1442: arc lint + arc unit

Event Timeline

aklapper created this revision.Jul 19 2024, 13:29

Owners added a reviewer: O1: Blessed Committers.Jul 19 2024, 13:29

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptJul 19 2024, 13:29

aklapper requested review of this revision.Jul 19 2024, 13:29

Harbormaster completed remote builds in B1441: Diff 2250.Jul 19 2024, 13:29

Note that every unmanaged exception is logged. So if you don't see a log, something is already managing this.

Better to see who is calling getConditionsForField() that has probably a try-catch that should have a phlog() but hasn't

In short, requireFieldImplementation seems correct in their legacy version, and something is not ideal in the consumer, that has a try catch. That try-catch should be edited.

Marking as "probably something different should be done"

This revision now requires changes to proceed.Jul 19 2024, 13:49

Per Valerio's previous comments

Harbormaster completed remote builds in B1442: Diff 2251.Jul 19 2024, 14:54

Hmm I guess now my patch title is too narrow as it logs any Herald exceptions? :D

Strip block

Seems super nice. Minor comment. If we do this, the stack trace is NOT shared:

phlog(pht('An exception occurred executing a Herald rule: "%s" Review '.
  'the Herald transcripts and correct or disable the problematic rule.',
  $ex->getMessage()));

If we do this instead (or something similar), the stack trace is shared:

phlog(pht('An exception occurred executing a Herald rule. Review '.
  'the Herald transcripts and correct or disable the problematic rule.'),
  $ex);

Untested. Still, thanks, seems lovely

valerio.bozzolan awarded a token.Jul 19 2024, 16:19

I personally don't see value in sharing a stacktrace in this case. YMMV :)

sgtm

This revision is now accepted and ready to land.Jul 20 2024, 08:22

aklapper mentioned this in T15869: Herald rule referring to a disabled rule silently fails.Jul 21 2024, 17:17

aklapper mentioned this in D25736: Log Herald rules having disabled Herald rules as condition via phlog().Jul 21 2024, 17:21

valerio.bozzolan added inline comments.Jul 21 2024, 18:04

src/applications/herald/engine/HeraldEngine.php
539	You are right from the other diff, here it would be probably better `$caught`, not `$ex`

Use $caught, not $ex.
Include Herald rule monogram in log message for easier finding.

Harbormaster completed remote builds in B1444: Diff 2256.Jul 21 2024, 18:09

aklapper marked an inline comment as done.Jul 21 2024, 18:14

aklapper mentioned this in Z1: Phorge.Aug 1 2024, 12:32

aklapper edited the summary of this revision. (Show Details)Aug 4 2024, 09:32

Closed by commit rPf6302c7ee300: Log Herald rules with invalid keys via phlog(). · Explain WhyAug 4 2024, 09:35

This revision was automatically updated to reflect the committed changes.

aklapper added a commit: rPf6302c7ee300: Log Herald rules with invalid keys via phlog().

aklapper mentioned this in rP71615f4b9917: Log Herald rules having disabled Herald rules as condition via phlog().

aklapper mentioned this in Next Up.Aug 12 2024, 09:21

valerio.bozzolan mentioned this in 2024 week 35.Aug 28 2024, 10:19

Revision Contents
Changeset List

Path

Size

Coverage (All)

Coverage (Touched)

Packages

src/

applications/

herald/

engine/

HeraldEngine.php

3 lines

O1: Blessed Committers

Diff 2251

View Options

src/applications/herald/engine/HeraldEngine.php

Show All 24 Lines

public function setOldCommit($old_commit) {

$this->oldCommit = $old_commit;

return $this;

}

public function getOldCommit() {

return $this->oldCommit;

}

/**

* @return string|null Path to previous filename

public function loadOldFilename() {

$drequest = $this->request;

$repository_id = $drequest->getRepository()->getID();

$conn_r = id(new PhabricatorRepository())->establishConnection('r');

$commit_id = $this->loadCommitId($this->oldCommit);

$old_commit_sequence = $this->loadCommitSequence($commit_id);

$path = '/'.$drequest->getPath();

$old_commit_sequence = $this->loadCommitSequence($commit_id);

if ($old_commit_sequence === null) {

return null;

}

$commit_id = $this->loadCommitId($drequest->getCommit());

do {

$commit_sequence = $this->loadCommitSequence($commit_id);

$change = queryfx_one(

$conn_r,

'SELECT pc.changeType, pc.targetCommitID, tp.path

FROM %T p

Show All 31 Lines

private function loadCommitId($commit_identifier) {

$commit = id(new DiffusionCommitQuery())

->setViewer($this->viewer)

->withIdentifiers(array($commit_identifier))

->withRepository($this->request->getRepository())

->executeOne();

return $commit->getID();

}

/**

* @return string|null commitSequence ID

private function loadCommitSequence($commit_id) {

$conn_r = id(new PhabricatorRepository())->establishConnection('r');

$path_change = queryfx_one(

$conn_r,

'SELECT commitSequence

FROM %T

WHERE repositoryID = %d AND commitID = %d

LIMIT 1',

PhabricatorRepository::TABLE_PATHCHANGE,

$this->request->getRepository()->getID(),

$commit_id);

if ($path_change === null) {

return null;

}

return reset($path_change);

}

valerio.bozzolanUnsubmitted

Done

$commit_id);

- if ($path_change !== null) {

- return reset($path_change);

- }

- return null;

- }

+ if ($path_change === null) {

+ return null;

+ }

+ return $path_change;

+ }

}

With an early return null, we can change less lines

(Non-blocking tip. I still need to reproduce and test)

valerio.bozzolan: With an early return null, we can change less lines (Non-blocking tip. I still need to…

}

Log Herald rules with invalid keys via phlog()ClosedPublicActions