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 Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4862 +/- ##
==========================================
- Coverage 95.01% 93.28% -1.73%
==========================================
Files 303 255 -48
Lines 7950 7123 -827
Branches 1608 1471 -137
==========================================
- Hits 7554 6645 -909
- Misses 396 478 +82
🚀 New features to boost your workflow:
|
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. |
|
How are things coming with the sdk-logs package? I don't know where the best place to look is for information about that. |
|
Now that |
We'll do this in a feature-release follow-up once we've published 2.0. All focus for now should be on SDK 2.0 and making any necessary breaking changes - the window for that will close mid-February #5148. We'll shift focus on logs once we pick it as a focus topic, see selection criteria here #5149 |
|
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
|
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
…try/api as experimental
hectorhdzg
force-pushed
the
hectorhdzg/logsapiintgr
branch
from
225df00 to
d31a671
Compare
|
@trentm and @pichlermarc this one is ready for another round of reviews, because the project is using more modern typescript and node.js runtime having the import from experimental path is fully supported now |
|
There are still some changes that @svetlanabrennan has been looking into for the logs API. It would be nice to get them in before this is merged, but I'll take a look at this and give it a review. |
|
This PR is blocked on at least #5733 and some other issues https://github.com/open-telemetry/opentelemetry-js/milestone/19. Then we can go ahead and merge the Logs API into |
6 participants
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.