Skip to content

Discussion: Criteria for upgrading diagnostic reporting from experimental to stable #26293

Closed
#32242
Closed
Discussion: Criteria for upgrading diagnostic reporting from experimental to stable#26293
#32242
Labels
experimentalIssues and PRs related to experimental features.reportIssues and PRs related to Node.js diagnostic reports.

Description

@cjihrig
cjihrig
opened on Feb 24, 2019

I don't think it's too early to start discussing the criteria for eventually moving the new diagnostic reporting functionality out of experimental status. As a diagnostic tool, the criteria might be different from n-api, http2, or workers, as the ecosystem is less likely (though not impossible) to build other modules on top of the core functionality.

Activity

  1. gireeshpunathil commented on Feb 25, 2019

    @gireeshpunathil
    gireeshpunathil
    on Feb 25, 2019
    Member

    @cjihrig - here are few questions / considerations that I can think of:

    • user interfaces (flags, API semantics): are they consumable in the current form, and ratified by 'some' real users? [ my take is that we should; may be evangelize through few conferences and then collect some feedback ]
    • or aret those need to be ratified by end users, instead our conviction is sufficient?
    • platform coverage: I guess we don't have any platform skipped for report that Node.js is supported.
    • test coverage: I don't know the latest on this, but there were few new flags that came up which did not have coverage, when I ported it.
    • worker support: do we need to support workers, to be able to call it stable? [ my take is we don't need to, this is something we can do later without changing the external interfaces ]
    • if so, what does it mean by worker support? many of the sections in the report are process-scopped (versions, command line, shared libs, etc.) while few are environment / isolate scopped (call stack, JS heap, uv loop etc.). A full worker support in my opinion would be: those sections should have an array of artifacts, covering all the workers and the main. In addition, worker data inclusion could be configurable.
    • should the unhandled native crashes (GPF) be supported under fatalexception? Right now this is not supported, as GPF handling requires signal safe operations, and report needs to be heavily refactored. [ my take is that it depends on the number of such cases we get, as well as end user suggestions ].
    • right now there is a bug in the report. When fatal error occurs with OOM, a report is generated irrespective of the configuration. This is because the env data where we store the config data is unavailabe in fatal error contexts; so we cannot figure out what the user request was. This could be solved in a couple of ways, but worker support decision can potentially influence how we want to attack this.

    Overall, I agree that it is reasonable to discuss the experimental exit criteria at this point, and identify / action on items that we agree upon.

  2. addaleax
    added
    experimentalIssues and PRs related to experimental features.
    reportIssues and PRs related to Node.js diagnostic reports.
    on Mar 13, 2019

  3. cjihrig commented on Apr 3, 2019

    @cjihrig
    cjihrig
    on Apr 3, 2019
    ContributorAuthor

    I think this list can be condensed a bit down to:

    • User adoption. Again, I don't think many modules will be built on top of this feature.
    • Test coverage. I checked the coverage, and I think this metric is beyond good enough for classification as stable. Some code paths that are covered are not reported in our coverage reports.
    • Handling of GPF and OOM crashes. I would classify these as bugs, but not blocking bugs.
    • Worker support.

    I'd really like to see this move out of experimental before Node 12 goes LTS.

  4. richardlau commented on Jun 7, 2019

    @richardlau
    richardlau
    on Jun 7, 2019
    Member

    nodejs/diagnostics#306 proposing versioning the report format.

  5. cjihrig commented on Jun 7, 2019

    @cjihrig
    cjihrig
    on Jun 7, 2019
    ContributorAuthor

    Proof of concept for the versioning thing at #28121.

  6. richardlau commented on Jan 17, 2020

    @richardlau
    richardlau
    on Jan 17, 2020
    Member

    Support for Workers: #31386

  7. cjihrig commented on Feb 9, 2020

    @cjihrig
    cjihrig
    on Feb 9, 2020
    ContributorAuthor

    I'm working on unflagging this. These are the changes I'm implementing:

    • Make the node_report configure option a no-op. The idea is that report will always be available moving forward.
    • Make the --experimental-report CLI flag a no-op and remove its uses in the code and docs. The experimental warning will also be removed.
    • Make the --report-on-fatalerror CLI flag experimental. It seems like most remaining issues are related to this flag and things like OOM scenarios. I don't think it's worth keeping everything else experimental because of this.

    I'll follow up with semver-major changes:

    • Remove the no-op node_report configure option.
    • Remove the no-op --experimental-report CLI flag.

    Is anyone opposed to this plan? It seems like the report feature is mostly stable at this point, and it would be great to get more people using it by unflagging.

  8. sam-github commented on Feb 9, 2020

    @sam-github
    sam-github
    on Feb 9, 2020
    Contributor

    LGTM, but consider leaving the no-op CLI flag around for as long as its needed in some supported Node.js versions, this seems to be the pattern we're using for other options (--http-parser) that don't do anything, but are still allowed to be specified so the same invocation works for across all release lines.

  9. legendecas commented on Feb 10, 2020

    @legendecas
    legendecas
    on Feb 10, 2020
    Member
    • Make the --experimental-report CLI flag a no-op and remove its uses in the code and docs. The experimental warning will also be removed.

    Then would an opt-out option to disable the generation of node-report make sense with it?

  10. cjihrig commented on Feb 10, 2020

    @cjihrig
    cjihrig
    on Feb 10, 2020
    ContributorAuthor

    Then would an opt-out option to disable the generation of node-report make sense with it?

    I might be misunderstanding your comment, but I don't think that's necessary. --experimental-report currently enables the diagnostic report feature, but the reports should only be generated if --report-uncaught-exception, --report-on-signal, --report-on-fatalerror, etc. is used.

  11. gireeshpunathil commented on Mar 11, 2020

    @gireeshpunathil
    gireeshpunathil
    on Mar 11, 2020
    Member

    @cjihrig -

    v14.0.0 is being planned with March 24th as the development-cut date #32181 . It would be great to release report as stable in 14, given it is a major plus slated for future LTS. Are you already working on items listed in #26293 (comment) ? let me know if I can be of any help!

  12. cjihrig commented on Mar 11, 2020

    @cjihrig
    cjihrig
    on Mar 11, 2020
    ContributorAuthor

    Yes, my plan is to have this ready to land as stable in Node 14.

  13. legendecas commented on Mar 11, 2020

    @legendecas
    legendecas
    on Mar 11, 2020
    Member

    Make the --report-on-fatalerror CLI flag experimental. It seems like most remaining issues are related to this flag and things like OOM scenarios. I don't think it's worth keeping everything else experimental because of this.

    Just bringing #29881 up to see if --report-on-fatalerror not been respected (conditional generating reports on fatal error regardless of the presence of the flag) will be counted as a blocker to the stable promotion?

  14. gireeshpunathil commented on Mar 11, 2020

    @gireeshpunathil
    gireeshpunathil
    on Mar 11, 2020
    Member

    @legendecas - as quoted somewhere, the issue #29881 is trying to address is a bug, not a missing feature, so should not be a blocker for elevation to stable.

    However, I don't see major challenges in getting that addressed in 2 week's time as well!

  15. 28 remaining items

  16. MylesBorins
    added 3 commits that reference this issue on Mar 24, 2020

    test: remove common.skipIfReportDisabled()

    76781bd

    doc,lib,src,test: make --experimental-report a nop

    cd388b2

    doc: update stability of report features

    2e1fb2b
  17. targos
    added 12 commits that reference this issue on Apr 25, 2020

    src: unconditionally include report feature

    2939a45

    build: remove node_report option in node.gyp

    e895baf

    build: make --without-report a no-op

    bda6d4c

    test: remove common.skipIfReportDisabled()

    38ef658

    doc,lib,src,test: make --experimental-report a nop

    45c8c2d

    doc: update stability of report features

    5587d66

    src: unconditionally include report feature

    0b310df

    build: remove node_report option in node.gyp

    70eaba1

    build: make --without-report a no-op

    984ae30

    test: remove common.skipIfReportDisabled()

    6a0bc83

    doc,lib,src,test: make --experimental-report a nop

    90d35ad

    doc: update stability of report features

    fd2486e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    experimentalIssues and PRs related to experimental features.reportIssues and PRs related to Node.js diagnostic reports.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions