feat(api): Integrate @opentelemetry/api-logs package into @opentelemetry/api as experimental #4862
Conversation
|
@open-telemetry/javascript-approvers please take a look, this should be really straightforward, is just moving code around and reusing util code that was duplicated. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4862 +/- ##
==========================================
+ Coverage 91.04% 92.10% +1.06%
==========================================
Files 89 104 +15
Lines 1954 2204 +250
Branches 416 437 +21
==========================================
+ Hits 1779 2030 +251
+ Misses 175 174 -1
|
Why delete in a separate PR? If they were done in the same commit, then git would (most likely) recognize the files as having been moved. Then later |
There was a problem hiding this comment.
From @dyladan's comment: #4400 (comment)
for this to be loadable via @opentelemetry/api/experimental or @opentelemetry/api/experimental/logs or something like that, he means not having these exports in the top-level index.ts.
Instead:
- They would have to be exported by src/experimental/index.ts, if Dan meant usage something like
const { logs, SeverityNumber } = require('@opentelemetry/api/experimental');, or - They would have to be exported via a new entry in "exports" in "package.json", if Dan meant usage something like
const { logs, SeverityNumber } = require('@opentelemetry/api/experimental/logs');. (Note the additional "/logs" on the argument torequire().
@dyladan Had you meant a particular one of those usages?
I favour option 1, because with option 2 I think '@opentelemetry/api/experimental/logs' as an entry-point is a little over-wordy and getting a logs export name from an entry-point ending in "/logs" is rife for confusion.
A mini-test for usage of option 1:
use-logs.js
console.log('-- using "trace"-y stuff from @opentelemetry/api for comparison');
var { trace, SpanKind } = require('@opentelemetry/api');
console.log('trace.getSpan: ', trace.getSpan);
console.log('SpanKind.SERVER: ', SpanKind.SERVER);
console.log('\n-- using @opentelemetry/api-logs');
var { logs, SeverityNumber } = require('@opentelemetry/api-logs');
console.log('logs.getLogger: ', logs.getLogger);
console.log('SeverityNumber.WARN: ', SeverityNumber.WARN);
console.log('\n-- using @opentelemetry/api/experimental');
var { logs, SeverityNumber } = require('@opentelemetry/api/experimental');
console.log('logs.getLogger: ', logs.getLogger);
console.log('SeverityNumber.WARN: ', SeverityNumber.WARN);For that last block to work would take a starter patch like this:
diff --git a/api/src/experimental/index.ts b/api/src/experimental/index.ts
index a05c7ba21..04fc85910 100644
--- a/api/src/experimental/index.ts
+++ b/api/src/experimental/index.ts
@@ -15,3 +15,18 @@
*/
export { wrapTracer, SugaredTracer } from './trace/SugaredTracer';
export { SugaredSpanOptions } from './trace/SugaredOptions';
+
+// Logs
+export { Logger } from './logs';
+export { LoggerProvider } from './logs';
+export { LogRecord } from './logs';
+export { LogBody } from './logs';
+export { SeverityNumber } from './logs';
+export { LoggerOptions } from './logs';
+export { AnyValue } from './logs';
+export { AnyValueMap } from './logs';
+export { NoopLogger } from './logs';
+export { NoopLoggerProvider } from './logs';
+export type { LogsAPI } from './logs';
+
+export { logs } from './logs';
diff --git a/api/src/index.ts b/api/src/index.ts
index 1d96a3052..af483a9a4 100644
--- a/api/src/index.ts
+++ b/api/src/index.ts
@@ -103,19 +103,6 @@ export {
} from './trace/invalid-span-constants';
export type { TraceAPI } from './api/trace';
-// Logs
-export { Logger } from './experimental/logs';
-export { LoggerProvider } from './experimental/logs';
-export { LogRecord } from './experimental/logs';
-export { LogBody } from './experimental/logs';
-export { SeverityNumber } from './experimental/logs';
-export { LoggerOptions } from './experimental/logs';
-export { AnyValue } from './experimental/logs';
-export { AnyValueMap } from './experimental/logs';
-export { NoopLogger } from './experimental/logs';
-export { NoopLoggerProvider } from './experimental/logs';
-export type { LogsAPI } from './experimental/logs';
-
// Split module-level variable definition into separate files to allow
// tree-shaking on each api instance.
import { context } from './context-api';
@@ -123,10 +110,9 @@ import { diag } from './diag-api';
import { metrics } from './metrics-api';
import { propagation } from './propagation-api';
import { trace } from './trace-api';
-import { logs } from './experimental/logs';
// Named export.
-export { context, diag, metrics, propagation, trace, logs };
+export { context, diag, metrics, propagation, trace };
// Default export.
export default {
context,
@@ -134,5 +120,4 @@ export default {
metrics,
propagation,
trace,
- logs,
};Though, I think that could be refined a bit to avoid having the "src/experimental/logs/index.ts" file at all, to avoid one extra level of export *s -- similar to how there is no "src/trace/index.ts".
There was a problem hiding this comment.
package.json "exports" needed to be able to do import like "@opentelemetry/api/experimental" are supported in typescript > 4.7, and it also needs module setting in tsconfig to be updated to node16 or nodenext instead of commonjs used in the project, do we want to move forward with that kind of change? @trentm @pichlermarc
I can also update import for experimental code to "require" calls instead to avoid typescript to complain, let me know what you think.
There was a problem hiding this comment.
Current, typescript do not like unless updated
import { logs, NoopLogger } from '@opentelemetry/api/experimental';New
const { logs, NoopLogger } = require('@opentelemetry/api/experimental');There was a problem hiding this comment.
Q1: To clarify the limitation here: Does this mean that a user of Node.js version less than 16 and TypeScript less than 4.7 is not able to use any package that uses entry-points (as defined by "exports" in package.json)?
IIUC, options are:
- Hold this PR until SDK 2.0 (https://github.com/open-telemetry/opentelemetry-js/milestone/17) when the Node.js and TypeScript min verisons are bumped. Q: Does this even help? "SDK 2.0" is about a new major version of the SDK packages, i.e. not about the API package which we are talking about? I hope we aren't stuck with having to support Node.js 8 and TypeScript 4.4 as min-supported versions for the API package forever.
- Consider bumping the min-support TypeScript (currently 4.4) to 4.7, including for the API package, without a major version bump. Q: Does this fully solve the issue? Are users of Node.js less than 16 still stuck being able to use
@opentelemetry/api/experimentalat all? - Document that there are limitations in using
@opentelemetry/api/experimentalfor users of TypeScript>=4.4 <4.7, and move forward with this PR. I asked "Q1" above to clarify what the limitations actually are, to help decide if they would be acceptable.
There was a problem hiding this comment.
(For the record, here is the TS 4.7 release notes reference about adding "exports" support: https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/#package-json-exports-imports-and-self-referencing)
There was a problem hiding this comment.
Packaje.json exports are supported since Node.js 12
https://nodejs.org/en/blog/release/v12.7.0
But like you mentioned typescript added support in 4.7, not sure why node16 and nodenext are needed as well as module setting, I was not able to make it work without using those. TS output was mentioning that explicitly, I cannot find any documentation why that is needed, but I will keep digging.
There was a problem hiding this comment.
Users using Node.js < 12.7 and/or typescript <4.7 will not be able to use package.json exports, we can wait for SDK 2, I'm just worried that could be mean to have logs stable delayed by a lot of time, using experimental in "api" package was proposed for transition purpose only, it would be worth considering to conduct the specification review in our api-logs package instead and move forward to add it in "api" fully without experimental path. Adding @dyladan who proposed having it in experimental path in the first place.
There was a problem hiding this comment.
Notes from discussions today:
- The TypeScript 4.4 limitation with entry-points can apparently be solves by also including a
typesVersionsentry in the package.json. This is being used in the recent addition of the "./incubating" entry-point in the semconv package: - On the Node < 12.7 limitation: This could either (a) be ignored (users of those old Node.js versions just cannot use the newer functionality, but otherwise should not be broken), or (b) if absolutely necessary there is probably a hack to could be made to work via a top-level "experimental.js" that acts as the fake entry-point for earlier Node.js versions that don't support entry-points.
From discussion in the OTel JS SIG today, the favoured option for now is to pursue TC review of the Logs API (not sure about the Logs SDK) with plans to then stabilize the logs API and just have it in the usual @opentelemetry/api export.
@trentm I was trying to make it easier for review having the updated files below the hundreds :) is fine for me I will include all changes here. |
Whoa, will there be 100s? :) I suppose there will be the delete of all the meta files like |
|
We discussed introducing these changes would cause issues to users using Node.js < 12.7 and/or typescript <4.7, there is a proposal to skip this changes and move logs API directly to API package once formal review is done. |
Fixes #4400
Migrated api logs package functionality to api package as experimental,
My plan is to delete older project as a different PR, same as updating dependencies using api-logs in other packages.