hicommonwealth / commonwealth

A platform for decentralized communities
https://commonwealth.im
GNU General Public License v3.0
64 stars 38 forks source link

Changed heroku startup so that we use node instead of ts-node #5001

Closed kurtisassad closed 4 months ago

kurtisassad commented 8 months ago

Link to Issue

Closes: #3051

Description of Changes

Test Plan

  1. Ran on frick
  2. Load test (Nakul's load test PR) on this commit:
      "http.response_time": {
        "min": 218,
        "max": 3038,
        "count": 872,
        "p50": 658.6,
        "median": 658.6,
        "p75": 1002.4,
        "p90": 1465.9,
        "p95": 1826.6,
        "p99": 2416.8,
        "p999": 2893.5
      },
  3. Ran master on frick
  4. Load test on master (note median response time went up by 200ms) My guess is that the server is doing more page swaps when loading memory from the db causing it to hang longer than normal:
    "http.response_time": {
        "min": 220,
        "max": 4147,
        "count": 875,
        "p50": 854.2,
        "median": 854.2,
        "p75": 1495.5,
        "p90": 2059.5,
        "p95": 2671,
        "p99": 3678.4,
        "p999": 4065.2
      },

Rollbar test (make sure that source maps work correctly)

  1. Put error in status call
  2. Start up server
  3. Navigate to localhost:8080, check rollbar and make sure error stack trace points to correct locations in ts files.

Deployment Plan

  1. No changes needed by leads, since the heroku build (yarn build-all) and deployment (procfile web) in this PR has been changed.

Other Considerations

jnaviask commented 8 months ago

@kurtisassad is this usable locally? how can I run it?

kurtisassad commented 8 months ago

@jnaviask I can add a package.json script for it, but if we determine it is not necessary just run:

yarn install
yarn build-app
NODE_ENV=production NO_PRERENDER=true NO_SSL=true NODE_OPTIONS=--max-old-space-size=$(../../scripts/get-max-old-space-size.sh) node ./build/commonwealth/server.js

2nd command is the heroku build step 3rd command is the heroku run step (defined in procfile) with some of the features disabled (SSL, prerender)

jnaviask commented 8 months ago

tagging @timolegros for second opinion code review here -- then we should be good to go

kurtisassad commented 8 months ago

Good points Tim. I will draft this PR and work on cleaning up our tsconfigs/transpile steps so that we do the minimal amount of transpiling. I will work on finishing this up for today, and keep it in draft until ready.

kurtisassad commented 8 months ago

tsconfig.consumer.json needed to run the consumer locally (via ts-node).

If we decide it is worthwhile, I would like to make a fast follow to achieve the ideal slug. Basically this PR introduces more slug size, because instead of running the typescript files directly, we transpile them into javascript. So the result is:

master slug (435 MB) = all packages(ts) + bundle(js) this PR slug (435.3 MB) = all packages(ts) + bundle(js) + server(js) the ideal slug = bundle(js) + server(js)

the slug size of this PR is only increased by 0.3MB (compressed) so I think it is not an issue, since we are still well below the limit

kurtisassad commented 5 months ago

Drafting while I add source maps and test to make sure they are working on our data analytics platforms

kurtisassad commented 5 months ago

Source maps works, tested with rollbar, stack traces correctly point to location in app (ts file) where it fails.

Easy workaround was to add the --enable-source-maps flag to node + add option to production tsconfig. This makes it just map the source maps when building error stack traces, as a result, no complicated steps need to be taken in our 3rd party integrations.

timolegros commented 5 months ago

Source maps works, tested with rollbar, stack traces correctly point to location in app (ts file) where it fails.

Easy workaround was to add the --enable-source-maps flag to node + add option to production tsconfig. This makes it just map the source maps when building error stack traces, as a result, no complicated steps need to be taken in our 3rd party integrations.

Does this diminish the performance improvements we get and does it significantly impact slug size?

kurtisassad commented 5 months ago

Drafting while looking into Tims comment

kurtisassad commented 5 months ago

The slug size is increased 0.5MB. (348.5 -> 349MB)

For performance, using Nakul's Load testing PR I get: Without source maps:

      "http.response_time": {
        "min": 201,
        "max": 4281,
        "count": 908,
        "p50": 399.5,
        "median": 399.5,
        "p75": 837.3,
        "p90": 1525.7,
        "p95": 2101.1,
        "p99": 3328.3,
        "p999": 4231.1
      },

With source maps:

      "http.response_time": {
        "min": 201,
        "max": 4822,
        "count": 872,
        "p50": 424.2,
        "median": 424.2,
        "p75": 804.5,
        "p90": 1686.1,
        "p95": 2322.1,
        "p99": 3134.5,
        "p999": 4583.6
      },

So it does look like there is a minor performance improvement without source maps, but I am not sure how statistically significant this is.

kurtisassad commented 5 months ago

Re-ran load test for master on frack to update stats. Got:

      "http.response_time": {
        "min": 208,
        "max": 3722,
        "count": 908,
        "p50": 541.5,
        "median": 541.5,
        "p75": 842.6,
        "p90": 1300.1,
        "p95": 1901.1,
        "p99": 2595.5,
        "p999": 2798.4
      },
timolegros commented 5 months ago

This should probably be merged and deployed as a separate release so we can easily rollback in the event of a failure (not that I expect a failure) @jnaviask @kurtisassad - thoughts?

kurtisassad commented 5 months ago

Separate release sounds good to me

Rotorsoft commented 4 months ago

Closing and recreating in #6296 due to large merge conflicts

Rotorsoft commented 4 months ago

See #6296