Http.createServer is missing the options variable #629

Pixelatex · 2020-05-06T07:51:18Z

As mentioned in this issue. The http.createServer function was missing the options that the original node function has.

I simply added the missing options.

* add live metrics (microsoft#494) * add QPS files; enablement not included * add tests; add qps ping retry walkback * add off by default config option for QPS * add qps test to ensure off by default * only construct qps when enabled * add qps Sender constants * add QuickPulseEnvelopeFactory * fix ts timer compile * add qps exception and trace error handling * refactor default live metrics test * make qps hostname configurable * fix node0 setTimeout error * add qps telemetrytype constants * move away from export enums for ts backcompat * add qps EventDocument,fix dependency duration * use envelope type in qps telemetryprocessor * send avg execution duration instead of last dur * add error logging for qps endpoint * add telemetry types, move qps config to qpssender * fix keying type issue in TelemetryType * fix wrong telemetrytype in qps conversion (microsoft#500) * bump version to 1.3.0 (microsoft#501) * bump version to 1.3.0 * add setSendLiveMetrics to README * livemetrics: add lastSendSucceeded to prevent permanent retries (microsoft#504) * add lastSendSucceeded to prevent permanent retries * add qps time constants, remove static class state * fix request averages being NaN * bump to 1.3.1 (microsoft#505) * update diagnostic-channel-publishers to 0.3.1 (microsoft#509) * update diagnostic-channel-publishers to 0.3.1 * updates for node12 compat * run functionals on node12 * only log extended duration QPS errors (microsoft#511) * only log extended duration QPS errors * refactor QPS error logging * cleanup QPS error logging * readme: document live metrics host config option * Fix typos in readme.md (microsoft#517) * parse nonstring request-context header (microsoft#521) * parse nonstring request-context header * refactor header parsing, add try/catch on fallback * move includeCorrelationHeader to Util * add node.js native metrics (microsoft#508) * release 1.3.0 (microsoft#502) * add live metrics (microsoft#494) * add QPS files; enablement not included * add tests; add qps ping retry walkback * add off by default config option for QPS * add qps test to ensure off by default * only construct qps when enabled * add qps Sender constants * add QuickPulseEnvelopeFactory * fix ts timer compile * add qps exception and trace error handling * refactor default live metrics test * make qps hostname configurable * fix node0 setTimeout error * add qps telemetrytype constants * move away from export enums for ts backcompat * add qps EventDocument,fix dependency duration * use envelope type in qps telemetryprocessor * send avg execution duration instead of last dur * add error logging for qps endpoint * add telemetry types, move qps config to qpssender * fix keying type issue in TelemetryType * fix wrong telemetrytype in qps conversion (microsoft#500) * bump version to 1.3.0 (microsoft#501) * bump version to 1.3.0 * add setSendLiveMetrics to README * add native metrics subscriber * mark native members of NativePerformance as static * remove segfault lib * update package.json * native: add throw tests * add refs to metrics * add config for enabling metrics, add heap metrics * report stdDev stats * rename custom metric names * fix native perf test * only run tests for node4+ * use node-pre-gyp native metrics * optionaldevdep for travis tests * simulate optionalDevDependency in travis * remove duped travis npm install * gate native metrics to node6+ * refactor extended metrics enablement * move extended metrics arg parsing to API * add info logging for when native module is loaded * docs: add extended metrics * add extended metrics build stage * use env var for travis extended metrics tests * test: modify test for non-installed case * tweak travis.yml * readme: comment out docs * ship mapfiles (microsoft#526) * ship mapfiles * tsconfig: inline TS in sourcemaps * qps: never change ping state on errors (microsoft#525) * qps: only pinging state for consecutive errors * qps: do not change state on errors * add w3c distributed tracing (microsoft#519) * add w3c distributed tracing * TraceParent/TraceState to Traceparent/Tracestate * traceparent lint errors * Rename rest of Tracestate/Traceparent * temp del * re-add Traceparent/Tracestate * tests: remove ambiguous url * refactor traceparent parsing in HttpRequestParser * add if guard on unneeded request parsing * trim traceparent header only once * disable qps only on consec errors * revert qps change * resolve merge conflicts * use enum for w3c enablement * remove dead code * move isValidW3CId to Util * create traceparent if tracestate exists * tests: add util import to httprequestparser * bump to 1.4.0 (microsoft#527) * docs: add w3c to readme (microsoft#528) * docs: add w3c to readme * docs: add default tracing mode * release 1.3.0 (microsoft#502) (microsoft#530) * add live metrics (microsoft#494) * add QPS files; enablement not included * add tests; add qps ping retry walkback * add off by default config option for QPS * add qps test to ensure off by default * only construct qps when enabled * add qps Sender constants * add QuickPulseEnvelopeFactory * fix ts timer compile * add qps exception and trace error handling * refactor default live metrics test * make qps hostname configurable * fix node0 setTimeout error * add qps telemetrytype constants * move away from export enums for ts backcompat * add qps EventDocument,fix dependency duration * use envelope type in qps telemetryprocessor * send avg execution duration instead of last dur * add error logging for qps endpoint * add telemetry types, move qps config to qpssender * fix keying type issue in TelemetryType * fix wrong telemetrytype in qps conversion (microsoft#500) * bump version to 1.3.0 (microsoft#501) * bump version to 1.3.0 * add setSendLiveMetrics to README

markwolff · 2020-05-06T16:40:10Z

AutoCollection/HttpRequests.ts

@@ -147,9 +147,9 @@ class AutoCollectHttpRequests {
        }

        const originalHttpServer = http.createServer;
-        http.createServer = (onRequest) => {
+        http.createServer = (options: https.ServerOptions, onRequest?: Function) => {


I believe options is also optional. Also, options will be an optional Function if no 2nd arg is provided

Same with the https function though.

Also, this is how it's documented and works by node:

http.createServer([options][, requestListener])

The current definition specifies provides 2 overloads. 1 optional callback or options and 1 optional callback. This patch should handle options being that optional callback or the required options

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/f1008ba6f314c49abe8747e1da52ef684bce9d29/types/node/v12/http.d.ts#L355-L356

Correct! My bad! I added the overload

Awesome, thanks! Last thing would be that javascript does not actually support function overloading. Although Typescript "supports" it for types, this is all lost when compiled to usable javascript. A function can only be defined once, and it must "support" all of the possible overload shapes

https://www.typescriptlang.org/docs/handbook/functions.html#overloads

You can update to the min node typings you need. We have a separate test npm run backcompat which tests compiling on older node typings. I'll add v4 to this list before cutting a release for this.

Alright, I'll try to fix the issues but some are in AutoCollection/Exceptions for example and those might need refactoring to work with the new typings.

@markwolff there seem to be quite a few issues:

File: AutoCollection/Exceptions.ts
Line: 66~

this._exceptionListenerHandle = handle.bind(this, true, AutoCollectExceptions.UNCAUGHT_EXCEPTION_HANDLER_NAME); process.on(AutoCollectExceptions.UNCAUGHT_EXCEPTION_HANDLER_NAME, this._exceptionListenerHandle);

This listener is defined as (reThrow:boolean, error: Error) => void, but the type definitions are expecting the type to be:

type UncaughtExceptionListener = (error: Error) => void;

And no matter what I did to fix it. It kept nagging about the static variables.

adding 'as any' silenced the warning but not sure if this is a good solution
process.on(AutoCollectExceptions.UNCAUGHT_EXCEPTION_HANDLER_NAME as any, this._exceptionListenerHandle);

I'm not sure why as any there silences the problem, as that field is a string. We are binding the 1st arg of the handler to the rethrow, so from process.on perspective, we should be passing it a string and a (error: Error) => void type.

The typescript overloads don't seem to recognize public static variables as a variable, they try to match it to the signals pipe.

msftclas · 2020-05-11T09:38:22Z

All CLA requirements met.

hectorhdzg · 2021-01-08T23:57:33Z

@Pixelatex I updated the code slightly and checks are successful now, let me know if you have any comments. We will add integration tests with node.js > 9 using options parameter in another PR and update node types to latest as well.

This reverts commit 2605117.

…)"" This reverts commit 9a95598.

#720) * Use request.protocol when available in automatic dependency collection * Revert "Http.createServer is missing the options variable (#629)" This reverts commit 2605117. * Revert "Revert "Http.createServer is missing the options variable (#629)"" This reverts commit 9a95598.

markwolff and others added 2 commits June 4, 2019 11:53

Http.createServer is missing the options variable

cf445ba

markwolff reviewed May 6, 2020

View reviewed changes

Pixelatex added 2 commits May 8, 2020 09:04

Add overload for non-options createserver

97c1a1f

Temporary commit, should be ready?

baff5b0

hectorhdzg and others added 5 commits December 10, 2020 17:15

Merge branch 'develop' into develop

7f42058

Merge branch 'develop' into develop

794d850

Merge branch 'develop' into develop

07f750e

Merge remote-tracking branch 'upstream/master' into develop

9d8579f

Reverting unnecessary changes. updating overload logic

8beb022

hectorhdzg requested a review from xiao-lix January 8, 2021 23:17

Reverting extra space

0d153b8

xiao-lix approved these changes Jan 12, 2021

View reviewed changes

hectorhdzg merged commit 2605117 into microsoft:develop Jan 12, 2021

hectorhdzg added a commit that referenced this pull request Jan 14, 2021

Revert "Http.createServer is missing the options variable (#629)"

9a95598

This reverts commit 2605117.

hectorhdzg added a commit that referenced this pull request Jan 14, 2021

Revert "Revert "Http.createServer is missing the options variable (#629…

9681b22

…)"" This reverts commit 9a95598.

hectorhdzg mentioned this pull request Feb 10, 2021

Appinsights breaks node's http.createServer function #628

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Http.createServer is missing the options variable #629

Http.createServer is missing the options variable #629

Pixelatex commented May 6, 2020 •

edited

Loading

markwolff May 6, 2020 •

edited

Loading

Pixelatex May 7, 2020

markwolff May 7, 2020

Pixelatex May 8, 2020

markwolff May 8, 2020

markwolff May 11, 2020

Pixelatex May 12, 2020

Pixelatex May 12, 2020 •

edited

Loading

markwolff May 12, 2020

Pixelatex May 13, 2020

msftclas commented May 11, 2020 •

edited

Loading

hectorhdzg commented Jan 8, 2021

Http.createServer is missing the options variable #629

Http.createServer is missing the options variable #629

Conversation

Pixelatex commented May 6, 2020 • edited Loading

markwolff May 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pixelatex May 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msftclas commented May 11, 2020 • edited Loading

hectorhdzg commented Jan 8, 2021

Pixelatex commented May 6, 2020 •

edited

Loading

markwolff May 6, 2020 •

edited

Loading

Pixelatex May 12, 2020 •

edited

Loading

msftclas commented May 11, 2020 •

edited

Loading