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

ingest: Render captive core logs as Horizon logs #3189

Merged
merged 18 commits into from
Nov 9, 2020

Conversation

Shaptic
Copy link
Contributor

@Shaptic Shaptic commented Nov 3, 2020

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with name of package that is most changed in the PR, ex.
    services/friendbot, or all or doc if the changes are broad or impact many
    packages.

Thoroughness

  • This PR adds tests for the most critical parts of the new functionality or fixes.
  • I've updated any docs (developer docs, .md
    files, etc... affected by this change). Take a look in the docs folder for a given service,
    like this one.

Release planning

  • I've updated the relevant CHANGELOG (here for Horizon) if
    needed with deprecations, added features, breaking changes, and DB schema changes.
  • I've decided if this PR requires a new major/minor version according to
    semver, or if it's mainly a patch change. The PR is targeted at the next
    release branch if it's not a patch change.

What

Adds an optional *log.Entry instance to the stellarCoreRunner and CaptiveStellarCore types so that we can process Captive Core's logs and redirect them to the Horizon log with the appropriate formatting / level / etc. The runner appends a subservice=captive-core to the entries as outlined in #2669, making filtering easier.

I've mapped Error and Fatal log levels to Error, while everything else maps to its equivalent. This is obviously trivial to change if we want to prioritize differently.

Specifically, a log that used to look like this:

GATBN [History WARNING] No writable archives configured, history will not be written.

now looks like

WARN[2020-11-03T15:40:28.421-08:00] History: No writable archives configured, history will not be written.  pid=59341 service=ingest subservice=stellar-core

Why

This closes #2669, which was re-opened to treat Core output like Horizon logs.

Known Limitations

Obviously, if Core's output format changes, the regex may need to be updated.

@Shaptic Shaptic requested a review from a team November 3, 2020 23:17
@Shaptic Shaptic self-assigned this Nov 3, 2020
@cla-bot cla-bot bot added the cla: yes label Nov 3, 2020
@Shaptic Shaptic force-pushed the captive-core-errors branch from 2deec7e to 557fe71 Compare November 3, 2020 23:19
@Shaptic Shaptic force-pushed the captive-core-errors branch from 1376e57 to 03fe249 Compare November 4, 2020 18:26
@Shaptic
Copy link
Contributor Author

Shaptic commented Nov 4, 2020

All good points; I believe they're resolved now @tamirms.

Copy link
Contributor

@bartekn bartekn 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! Minor comments and improvements ideas.

ingest/ledgerbackend/stellar_core_runner.go Show resolved Hide resolved
ingest/ledgerbackend/stellar_core_runner.go Outdated Show resolved Hide resolved
@@ -182,6 +182,8 @@ func NewSystem(config Config) (System, error) {
cancel()
return nil, errors.Wrap(err, "error creating captive core backend")
}
captiveCoreBackend.SetLogger(log)
ledgerBackend = captiveCoreBackend
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: you can leave the old code and write:

ledgerBackend.(*ledgerbackend.CaptiveStellarCore).SetLogger(log)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though less lines, is it cleaner? As someone newish to Go, it's quite a bit harder to read, especially with the new change about name + subservice, it becomes:

ledgerBackend.(*ledgerbackend.CaptiveStellarCore).SetStellarCoreLogger(log.WithField("subservice", "stellar-core"))

😮

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right!

ingest/ledgerbackend/captive_core_backend.go Outdated Show resolved Hide resolved
}

levelRx := regexp.MustCompile(`\[(\w+) ([A-Z]+)\]`)
indices := levelRx.FindStringSubmatchIndex(line)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is too complicated. If you use FindSubmatch and rewrite the regexp like in https://play.golang.com/p/mYf-9iMkOCg you won't have to deal with cutting the string yourself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially did this, but the problem is that I still needed the index to trim the line at the end. To do the above, I'd unfortunately have to match twice; do we want to do this? The code is definitely cleaner but slightly less efficient; it's done in 7187374, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

but the problem is that I still needed the index to trim the line at the end

You don't have to if you change the regexp to G[A-Z]{4} \[(\w+) ([A-Z]+)\] (.*) (as in play.golang example above). The line remaining is also a group match so it's in [3].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦 of course; I didn't notice the last match.

ingest/ledgerbackend/stellar_core_runner.go Outdated Show resolved Hide resolved
@Shaptic Shaptic merged commit ef1e30d into stellar:master Nov 9, 2020
@Shaptic Shaptic deleted the captive-core-errors branch November 9, 2020 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve captive-core error reporting
3 participants