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

chore(a3p-integration): Improve log output #10485

Merged
merged 13 commits into from
Nov 20, 2024

Conversation

gibson042
Copy link
Member

@gibson042 gibson042 commented Nov 14, 2024

Description

Extracted from #10165 and best reviewed by commit.

  • Include relevant variable details (e.g., block height).
  • Omit output that doesn't convey new information "done", "reading", etc.).
  • Provide a common logging prefix for subtasks, inlining functions where relevant.
  • Remove endo/init/legacy.js from z:acceptance ava config to eliminate noisy Object <[Object: null prototype] {}> output.
  • Introduce a logRecord helper to concisely log possibly-remotable-bearing records and/or record entries.

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

n/a

Testing Considerations

n/a

Upgrade Considerations

n/a

@gibson042 gibson042 added the force:integration Force integration tests to run on PR label Nov 14, 2024
@gibson042 gibson042 requested a review from a team as a code owner November 14, 2024 19:09
Copy link

cloudflare-workers-and-pages bot commented Nov 14, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 5b2e20c
Status: ✅  Deploy successful!
Preview URL: https://458fdf60.agoric-sdk.pages.dev
Branch Preview URL: https://gibson-2024-11-a3p-integrati.agoric-sdk.pages.dev

View logs

@gibson042 gibson042 force-pushed the gibson-2024-11-a3p-integration-logging branch from 0ffeeb9 to 4d3822b Compare November 14, 2024 20:24
@gibson042 gibson042 marked this pull request as draft November 15, 2024 15:06
@gibson042 gibson042 force-pushed the gibson-2024-11-a3p-integration-logging branch 5 times, most recently from ebec543 to 42945e4 Compare November 16, 2024 19:50
@gibson042 gibson042 changed the title [WIP] chore(a3p-integration): Improve log output Nov 16, 2024
@gibson042 gibson042 force-pushed the gibson-2024-11-a3p-integration-logging branch 2 times, most recently from 5192642 to 85dfb59 Compare November 18, 2024 00:33
@gibson042 gibson042 marked this pull request as ready for review November 18, 2024 00:37
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

I'm afraid I don't know enough to approve.

* @param {bigint} numValInPercent
*/
const toRatio = (brand, numValInPercent) => {
const commonDenominator = AmountMath.make(brand, 10_000n);
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why the denominator is not 100n. But I suppose that pre-dates this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, I just moved it. 🤷

Comment on lines +68 to +95
export const logRecord = (label, data, log = console.log) => {
const entries = Array.isArray(data) ? [...data] : Object.entries(data);
/** @type {[labelKey: PropertyKey, valueKey: PropertyKey] | undefined} */
let shape;
for (let i = 0; i < entries.length; i += 1) {
let entry = entries[i];
if (!Array.isArray(entry)) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not confident that I can follow this logic. And I'm not quite sure what it's supposed to do.

I'm looking for examples in ci logs, but I see:

Integration tests / test-docker-build (pull_request) Skipped

... even though the force:integration label is present.

Some examples in the docstring might be nice. Or unit tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the docstring. Actual output is visible in a prior CI run: https://github.com/Agoric/agoric-sdk/actions/runs/11881729913/job/33106407896#step:10:4506

  ✔ swap into IST using agd with default gas (9s)
    ℹ METRICS  {
        anchorPoolBalance: {
          brand: Object @Alleged: BoardRemoteUSDC_axl brand {},
          value: 30010011n,
        },
        feePoolBalance: {
          brand: Object @Alleged: BoardRemoteIST brand {},
          value: 0n,
        },
        mintedPoolBalance: {
          brand: Object @Alleged: BoardRemoteIST brand {},
          value: 30010011n,
        },
        totalAnchorProvided: {
          brand: Object @Alleged: BoardRemoteUSDC_axl brand {},
          value: 0n,
        },
        totalMintedProvided: {
          brand: Object @Alleged: BoardRemoteIST brand {},
          value: 30010011n,
        },
      }
    ℹ BALANCES { [denom]: amount, ... } {
        'ibc/295548A78785A1007F232DE286149A6FF512F180AF5657780FC89C009E2C348F': '300000000',
        ubld: '20000000',
        uist: '250000',
      }

Copy link
Member

Choose a reason for hiding this comment

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

This is a lot of new code in psm-lib that isn't specific to PSM. I'm not opposed to landing it but please don't spend effort improving these deprecated utils. It would be better spent on @agoric/client-utils and getting psm-lib to use that.

incidentally, consider marking @deprecated all the functions in psm-lib that aren't about PSM per se.

a3p-integration/proposals/n:upgrade-next/package.json Outdated Show resolved Hide resolved
Comment on lines 80 to 81
"require": [
"@endo/init/legacy.js"
Copy link
Member

Choose a reason for hiding this comment

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

I should know more about why we were using endo legacy before approving this.

I'm also not sure about why use imports in every file rather than ava require. I sort of prefer it that way, but it hasn't been the norm here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The general reason seems to generally be JS assignment override mistake mitigation, albeit in a noisy way:

$ for import in @endo/init @endo/init/legacy.js; do
  echo
  node --input-type=module -e "
    import '$import';
    import { inspect } from 'node:util';
    console.log('$import');
    let MyObject = () => 'not implemented';
    try {
      MyObject = function MyObject(props = {}) {
        return new.target ? Object.assign(this, props) : new MyObject(props);
      };
      (MyObject.prototype = Object.create(Object.prototype)).constructor = MyObject;
    } catch (err) { console.log('ERROR', err.message); }
    const instance = MyObject({ foo: 'bar' });
    console.log(util.inspect({
      data: {
        object: {instance},
        map: new Map([['instance', instance]]),
      }
    }, { depth: 10 }));
  "
done

@endo/init
ERROR Cannot assign to read only property 'constructor' of object '[object Object]'
{
  data: {
    object: { instance: { foo: 'bar' } },
    map: Map(1) { 'instance' => { foo: 'bar' } }
  }
}

@endo/init/legacy.js
Object <[Object: null prototype] {}> {
  data: Object <[Object: null prototype] {}> {
    object: Object <[Object: null prototype] {}> {
      instance: MyObject { foo: 'bar' }
    },
    map: Map <Object <[Object: null prototype] {}>>(1) [Map] {
      'instance' => MyObject { foo: 'bar' }
    }
  }
}

But AFAICT, it is not helpful here.

Copy link
Member

@turadg turadg Nov 19, 2024

Choose a reason for hiding this comment

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

IIRC some import of axios or protobufjs prompted it. I didn't realize it had any DX downsides. Given that it impairs the console messages I agree we should avoid it.

Howeverplease keep the endo/init config in the Ava require so it's consistent across tests. Also, shouldn't it be @endo/init/debug.js for better output?

Copy link
Member Author

Choose a reason for hiding this comment

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

shouldn't it be @endo/init/debug.js for better output?

Good point; done.

@gibson042 gibson042 force-pushed the gibson-2024-11-a3p-integration-logging branch 3 times, most recently from a2f79ba to b6e49c9 Compare November 18, 2024 22:05
),
]);
console.log('charterAcceptOfferId', charterAcceptOfferId);
const { brands, instances } = await snapshotAgoricNames();
Copy link
Member

Choose a reason for hiding this comment

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

Let's not put any more effort into improving these functions that can be replaced by @agoric/client-utils.

Comment on lines +68 to +95
export const logRecord = (label, data, log = console.log) => {
const entries = Array.isArray(data) ? [...data] : Object.entries(data);
/** @type {[labelKey: PropertyKey, valueKey: PropertyKey] | undefined} */
let shape;
for (let i = 0; i < entries.length; i += 1) {
let entry = entries[i];
if (!Array.isArray(entry)) {
Copy link
Member

Choose a reason for hiding this comment

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

This is a lot of new code in psm-lib that isn't specific to PSM. I'm not opposed to landing it but please don't spend effort improving these deprecated utils. It would be better spent on @agoric/client-utils and getting psm-lib to use that.

incidentally, consider marking @deprecated all the functions in psm-lib that aren't about PSM per se.

a3p-integration/proposals/n:upgrade-next/agoric-tools.js Outdated Show resolved Hide resolved
a3p-integration/proposals/p:upgrade-19/package.json Outdated Show resolved Hide resolved
@gibson042 gibson042 force-pushed the gibson-2024-11-a3p-integration-logging branch 2 times, most recently from 48f8df2 to 7eb1564 Compare November 19, 2024 21:54
Comment on lines +36 to +37
// TODO @import {Ratio} from '@agoric/zoe'
/** @typedef {any} Ratio */
Copy link
Member

Choose a reason for hiding this comment

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

It's in ERTP now. Why wait?

Suggested change
// TODO @import {Ratio} from '@agoric/zoe'
/** @typedef {any} Ratio */
/** @import {Ratio} from '@agoric/ertp`;

(it might need src/types.js)

Copy link
Member Author

Choose a reason for hiding this comment

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

error TS2305: Module '"@agoric/ertp"' has no exported member 'Ratio'.
error TS2305: Module '"@agoric/ertp/src/types.js"' has no exported member 'Ratio'.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, the proposal must have an old version of ERTP. We'd have the one in agoric-sdk if this used the new 'yarn link' trick but it's not worth further churn on the PR.

a3p-integration/proposals/p:upgrade-19/package.json Outdated Show resolved Hide resolved
...in anticipation of improving log output
…nit/debug.js

This should eliminate noisy `Object <[Object: null prototype] {}>` output.
@gibson042 gibson042 force-pushed the gibson-2024-11-a3p-integration-logging branch from 916d2ca to 5b2e20c Compare November 20, 2024 03:53
@gibson042 gibson042 added the automerge:rebase Automatically rebase updates, then merge label Nov 20, 2024
@mergify mergify bot merged commit 7e54272 into master Nov 20, 2024
85 of 91 checks passed
@mergify mergify bot deleted the gibson-2024-11-a3p-integration-logging branch November 20, 2024 04:34
mergify bot added a commit that referenced this pull request Nov 20, 2024
…0533)

## Description
A final step that I forgot to include in #10485, so it's now a followup.

### Security Considerations
n/a

### Scaling Considerations
n/a

### Documentation Considerations
n/a

### Testing Considerations
Covered by CI.

### Upgrade Considerations
n/a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants