chore: use semver to determine API compatibility #6

dyladan · 2021-02-19T14:59:13Z

Fixes #5

codecov · 2021-02-19T15:00:59Z

Codecov Report

Merging #6 (dd6e41a) into main (53244ed) will increase coverage by 4.97%.
The diff coverage is 97.05%.

@@            Coverage Diff             @@
##             main       #6      +/-   ##
==========================================
+ Coverage   89.82%   94.79%   +4.97%     
==========================================
  Files          33       37       +4     
  Lines         452      519      +67     
  Branches       77       82       +5     
==========================================
+ Hits          406      492      +86     
+ Misses         46       27      -19

Impacted Files	Coverage Δ
src/internal/global-utils.ts	`90.00% <90.00%> (ø)`
src/api/context.ts	`96.42% <100.00%> (+2.67%)`	⬆️
src/api/diag.ts	`100.00% <100.00%> (ø)`
src/api/propagation.ts	`100.00% <100.00%> (+6.66%)`	⬆️
src/api/trace.ts	`95.83% <100.00%> (+2.97%)`	⬆️
src/diag/logLevel.ts	`100.00% <100.00%> (ø)`
src/diag/logger.ts	`100.00% <100.00%> (ø)`
src/internal/semver.ts	`100.00% <100.00%> (ø)`
backwards-compatability/node10/index.ts
backwards-compatability/node8/index.ts
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53244ed...a6ede59. Read the comment docs.

dyladan · 2021-02-19T15:33:35Z

@MSNev the biggest changes here are related to the diag logger because it behaves differently than the other APIs so I would appreciated a peek

src/api/diag.ts

dyladan · 2021-02-23T18:52:38Z

@MSNev While trying to get together an implementation that would not allow changing log levels in the global logger, I came across the test titled 'diag setLogLevel is ignored when using a specific logger'. It seems to me if the global log level is INFO and i create a filtered logger with the DEBUG level. I should be able to see debug level logs when I call it. But this test appears to assert the opposite? What is the point of creating a separate logger if not to get more specific logging from one component than what the global logger is set to?

edit: I resolved it by doing this. Can you please take a look? dyladan#1

MSNev · 2021-02-23T19:10:26Z

@MSNev Nev Wylie FTE While trying to get together an implementation that would not allow changing log levels in the global logger, I came across the test titled 'diag setLogLevel is ignored when using a specific logger'. It seems to me if the global log level is INFO and i create a filtered logger with the DEBUG level. I should be able to see debug level logs when I call it. But this test appears to assert the opposite? What is the point of creating a separate logger if not to get more specific logging from one component than what the global logger is set to?

edit: I resolved it by doing this. Can you please take a look? dyladan#1

The test is asserting that if you just create a new level of "X" but the user / application set the level higher globally AND you have created a filtered logger using the default (without specifying a logger), it will default to delegating the logging api.diag, which will then obey the global logging level request. And if you want to override the global level then you need to specify the specific logger, so you need to get the underlying (unfiltered logger) via api.diag.getLogger()

This is documented explicitly in the TS doc of the createLogLevelDIagLogger()
https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-api/src/diag/logLevel.ts#L67-L79

dyladan · 2021-02-23T19:29:07Z

@MSNev Nev Wylie FTE While trying to get together an implementation that would not allow changing log levels in the global logger, I came across the test titled 'diag setLogLevel is ignored when using a specific logger'. It seems to me if the global log level is INFO and i create a filtered logger with the DEBUG level. I should be able to see debug level logs when I call it. But this test appears to assert the opposite? What is the point of creating a separate logger if not to get more specific logging from one component than what the global logger is set to?
edit: I resolved it by doing this. Can you please take a look? dyladan#1

The test is asserting that if you just create a new level of "X" but the user / application set the level higher globally AND you have created a filtered logger using the default (without specifying a logger), it will default to delegating the logging api.diag, which will then obey the global logging level request. And if you want to override the global level then you need to specify the specific logger, so you need to get the underlying (unfiltered logger) via api.diag.getLogger()

This is documented explicitly in the TS doc of the createLogLevelDIagLogger()
https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-api/src/diag/logLevel.ts#L67-L79

That's quite confusing to me. I would expect that if I called createLogLevelDiagLogger with a level the returned logger would respect that level. With no logger passed, I would expect it to use the same destination as the global, but not necessarily respect the global level.

I preserved your behavior in dyladan#1 for now

MSNev · 2021-02-23T19:35:47Z

That's quite confusing to me. I would expect that if I called createLogLevelDiagLogger with a level the returned logger would respect that level. With no logger passed, I would expect it to use the same destination as the global, but not necessarily respect the global level.

Yes, I agree. It came about from a comment from Bart where if the user has set the global logging level to NONE then he did not expect that we (by default) would override their level request. But we still had code (now removed as part of the config) that wanted to override and set directly.

dyladan · 2021-02-23T19:37:33Z

@obecny do you mind if we reverse this behavior?

obecny · 2021-02-23T19:38:04Z

That's quite confusing to me. I would expect that if I called createLogLevelDiagLogger with a level the returned logger would respect that level. With no logger passed, I would expect it to use the same destination as the global, but not necessarily respect the global level.

Yes, I agree. It came about from a comment from Bart where if the user has set the global logging level to NONE then he did not expect that we (by default) would override their level request. But we still had code (now removed as part of the config) that wanted to override and set directly.

which comment ?

dyladan · 2021-02-23T19:58:56Z

which comment ?

I said:

I would expect that if I called createLogLevelDiagLogger with a level the returned logger would respect that level. With no logger passed, I would expect it to use the same destination as the global, but not necessarily respect the global level.

Nev said:

Yes, I agree. It came about from a comment from Bart where if the user has set the global logging level to NONE then he did not expect that we (by default) would override their level request. But we still had code (now removed as part of the config) that wanted to override and set directly.

Would you be ok if that was the behavior?

obecny · 2021-02-23T20:02:20Z

which comment ?

I said:

I would expect that if I called createLogLevelDiagLogger with a level the returned logger would respect that level. With no logger passed, I would expect it to use the same destination as the global, but not necessarily respect the global level.

Nev said:

Yes, I agree. It came about from a comment from Bart where if the user has set the global logging level to NONE then he did not expect that we (by default) would override their level request. But we still had code (now removed as part of the config) that wanted to override and set directly.

Would you be ok if that was the behavior?

@dyladan I would expect the behaviour you exactly described, if you pass some params to a function it should respect this params, otherwise it doesn't make sense to insert any params......, not sure why it would be different. But then @MSNev said that it is working differently because of my comment, so @MSNev which comment I did that it is working differently ???

MSNev · 2021-02-23T20:03:19Z

I found the original comment and sorry @obecny I miss-attributed (and slightly miss-remembered) it to you when it was actually @vmarchaud on this comment.

The comment was around having the "forcedInfo" and specifically this bit

i don't expect having log coming where i explicitly dont want them

open-telemetry/opentelemetry-js#1880 (comment)

So while this is in relation to forceInfo(), I read the inferred the implicit log level request vs setting and recalled it as I mentioned above.

dyladan · 2021-02-23T20:10:06Z

@vmarchaud do you have an opinion on this?

vmarchaud · 2021-02-23T20:29:58Z

The comment was around having the "forcedInfo" and specifically this bit

i don't expect having log coming where i explicitly dont want them

I was refering to the fact that we couldn't (from my understand of the proposal) silence "forcedInfo".
If i get correctly the problem here, we are talking about the situation where i would have set the level to INFO on the global diag api but someone created another logger with a different level, in this case should we respect the global level or the level provided when constructing the logger ?
I believe the correct way (and whats possible with other APIs) is to use the level provided by the constructor, so i agree with @dyladan.

I understand why it oppose with my previous comment, but i think we should never construct a diag logger in any sdk components, only users should use this, so we can't really have a situation where the logger is using a level that wasn't choosen by the user

MSNev · 2021-02-23T20:53:17Z

but i think we should never construct a diag logger in any sdk components, only users should use this, so we can't really have a situation where the logger is using a level that wasn't choosen by the user

I TOTALLY agree with this, having the constructors / helpers is a "nice' to have for SDK / component developers during development ONLY.

src/api/diag.ts

dyladan · 2021-02-23T21:19:53Z

.eslintrc.js

@@ -44,6 +44,7 @@ module.exports = {
            "files": ["test/**/*.ts"],
            "rules": {
                "no-empty": "off",
+                "@typescript-eslint/ban-ts-comment": "off",


We need ts comments for asserting a type check will fail. This allows it in tests only

dyladan · 2021-02-23T21:32:48Z

@MSNev I marked our previous conversation around globals as resolved because it was a long conversation largely about code that has been removed. If you still feel my use of globals is invalid please open a new comment thread. Otherwise I would appreciate another review now that we've done some iterating.

MSNev · 2021-02-23T21:53:23Z

src/api/diag.ts

@@ -95,28 +70,23 @@ export class DiagAPI implements DiagLogger {
    // DiagAPI specific functions

    self.getLogger = (): DiagLogger => {
-      // Return itself if no existing logger is defined (defaults effectively to a Noop)
-      return _logger;
+      return getGlobal('diag') || _noopLogger;


Thought: Do we even need this anymore?

Can we just have this as the getChildLogger(), as there is no need to call getLogger() to bypass the filtering (as it won't) and the DiagAPI is a logger so you don't need to call api.diag.getLogger().debug(...)

Especially, as this getLogger() was previously just here to return the child logger.

And this would mean that the usage on L56 would have to change.

Do we need the diag API to understand the concept of filtering at all? We could just have the DiagLogger interface, and require the user to construct a valid logger for their use case like diag.setLogger(logLevelFilteredLogger(level, console))

The approach I was taking is that users and creators of loggers shouldn't need to worry about log level filtering as this should be controlled by the DiagAPI and any initialization configuration (as it should only be set once).

So I prefer what you have done in the setLogger registerGlobal('diag', createLogLevelDiagLogger(logLevel, logger), true) as it continues to hide the complexity.

MSNev · 2021-02-23T22:00:01Z

src/diag/logger.ts

@@ -59,6 +59,10 @@ export interface DiagLogger {
  verbose: DiagLogFunction;
 }

+export interface FilteredDiagLogger extends DiagLogger {


Question: Do we really need a new interface for this?

What about just adding this as an optional function, while it makes the usage a little more tricky we have to do this now if we don't already know (because of type enforcement) that this isn't a filtered logger. And the usages of this should be minimal anyway.

Would also suggest that if you want to keep it the name might be better as ParentDiagLogger with it's function of getChildLogger

MSNev · 2021-02-23T22:09:38Z

src/api/diag.ts

+      logLevel: DiagLogLevel = DiagLogLevel.INFO
+    ) => {
+      logger = logger === self ? self.getLoggingDestination() : logger;
+      registerGlobal('diag', createLogLevelDiagLogger(logLevel, logger), true);


Might be worth adding a comment that we are allowing the override of the default to both replacing any existing logger AND to avoid any possible endless loop (because of someone refactoring the code as the existing implementation doesn't have an issue) -- just so it make anyone touching the code in the future to think about that scenario before just blindly changing it.

MSNev · 2021-02-23T22:17:41Z

test/diag/logLevel.test.ts

@@ -50,8 +50,7 @@ describe('LogLevelFilter DiagLogger', () => {

  beforeEach(() => {
    // Set no logger so that sinon doesn't complain about TypeError: Attempted to wrap xxxx which is already wrapped
-    diag.setLogger();
-    diag.setLogLevel(DiagLogLevel.INFO);
+    diag.disable();


You may still need to reset the global log level as part of this as I found when running tests locally and in the CI environment that the order of execution can pollute other tests when they don't manager (or expect) the logging level to have changed.

But I think this is clearer than the side-effect version of setting the logger to undefined.

Since the log level is no longer stored as a separate property but is just enclosed in the registered filtered logger, this drops the level as well.

Flarna · 2021-02-24T07:49:50Z

src/internal/global-utils.ts

+  if (!allowOverride && api[type]) {
+    // already registered an API of this type
+    diag.error(
+      String(


Isn't this just a complicated variant of prepending Error: ?
If you want the call stack you should use new Error('...').stack.

It seems also that this path is not covered by a test. I wonder where this log is sent to when it happens during registration of diag.

Ah yes I needed stack thanks.

It seems also that this path is not covered by a test. I wonder where this log is sent to when it happens during registration of diag.

I will add a test. If this happens during the registration of diag, it will be sent to whichever diag was already registered. We could send to both diag instances which may in some cases result in duplicate logs but in other cases might prevent the log from being swallowed by incorrect diag use.

edit: diag is the only global allowed to be registered again, this is the allowOverride parameter. This is done so that the log level or destination may be changed by the user in the event they are experiencing an issue. Do you think it would be more clear to remove this special case and require the user to explicitly call disable if they want to reregister?

Flarna · 2021-02-24T07:57:00Z

Maybe create a separate PR for the diag refactoring? At least for me it's hard to follow this PR here which says it's about semver checks but more or less all discussions and a large amount of the changes are about diagnostics.

Please update PR description in first message at some allow people to get some hints what this is good for without the need to read through the old PR.

dyladan · 2021-02-24T14:09:22Z

Maybe create a separate PR for the diag refactoring? At least for me it's hard to follow this PR here which says it's about semver checks but more or less all discussions and a large amount of the changes are about diagnostics.

Please update PR description in first message at some allow people to get some hints what this is good for without the need to read through the old PR.

Sorry, the diag refactoring was a requirement for this. I'll split it into another PR to make this one more directed.

dyladan · 2021-02-24T16:03:04Z

Closing this PR in order to have the diag refactor in its own PR. Creating a new PR which depends on that PR. I will keep this branch without deleting until I'm sure we don't need it.

Diag refactor: #9
New global semver PR: #10

dyladan requested review from Flarna, johnbley, legendecas, markwolff, mayurkale22, mwear, naseemkullah, obecny, OlivierAlbertini and vmarchaud as code owners February 19, 2021 14:59

chore: use semver to determine API compatibility

73bd4da

dyladan force-pushed the semver-compatibility branch from 1bbed91 to 73bd4da Compare February 19, 2021 15:00

dyladan added the enhancement New feature or request label Feb 19, 2021

chore: do not throw

3c9b9e7

dyladan mentioned this pull request Feb 19, 2021

feat!: use semver to determine API compatibility open-telemetry/opentelemetry-js#1772

Closed

MSNev reviewed Feb 19, 2021

View reviewed changes

src/api/diag.ts Outdated Show resolved Hide resolved

MSNev reviewed Feb 19, 2021

View reviewed changes

src/api/diag.ts Outdated Show resolved Hide resolved

MSNev reviewed Feb 19, 2021

View reviewed changes

src/api/diag.ts Show resolved Hide resolved

chore: only filtered logger is global

2b576fa

chore: fix typo

29e47ff

dyladan added 2 commits February 23, 2021 15:51

chore: always use level set by user

04dda0e

Merge pull request #1 from dyladan/semver-compatibility-no-level

d728462

MSNev reviewed Feb 23, 2021

View reviewed changes

src/api/diag.ts Outdated Show resolved Hide resolved

MSNev reviewed Feb 23, 2021

View reviewed changes

src/api/diag.ts Outdated Show resolved Hide resolved

chore: prevent infinite recursion

28b2f3e

dyladan commented Feb 23, 2021

View reviewed changes

MSNev reviewed Feb 23, 2021

View reviewed changes

chore: rename destination child

640c1ea

MSNev reviewed Feb 23, 2021

View reviewed changes

Flarna reviewed Feb 24, 2021

View reviewed changes

dyladan added 2 commits February 24, 2021 10:06

chore: review comments

ca1b381

chore: fix build and add fallback

a6ede59

This was referenced Feb 24, 2021

chore: refactor diag logger #9

Merged

Use semver to determine API compatibility #10

Merged

dyladan closed this Feb 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: use semver to determine API compatibility #6

chore: use semver to determine API compatibility #6

dyladan commented Feb 19, 2021

codecov bot commented Feb 19, 2021 •

edited

Loading

dyladan commented Feb 19, 2021

dyladan commented Feb 23, 2021 •

edited

Loading

MSNev commented Feb 23, 2021

dyladan commented Feb 23, 2021 •

edited

Loading

MSNev commented Feb 23, 2021

dyladan commented Feb 23, 2021

obecny commented Feb 23, 2021

dyladan commented Feb 23, 2021

obecny commented Feb 23, 2021

MSNev commented Feb 23, 2021 •

edited

Loading

dyladan commented Feb 23, 2021

vmarchaud commented Feb 23, 2021

MSNev commented Feb 23, 2021

dyladan Feb 23, 2021

dyladan commented Feb 23, 2021 •

edited

Loading

MSNev Feb 23, 2021

MSNev Feb 23, 2021

MSNev Feb 23, 2021

dyladan Feb 23, 2021

MSNev Feb 23, 2021

MSNev Feb 23, 2021

MSNev Feb 23, 2021

MSNev Feb 23, 2021

dyladan Feb 24, 2021

Flarna Feb 24, 2021

dyladan Feb 24, 2021 •

edited

Loading

Flarna commented Feb 24, 2021

dyladan commented Feb 24, 2021

dyladan commented Feb 24, 2021

chore: use semver to determine API compatibility #6

chore: use semver to determine API compatibility #6

Conversation

dyladan commented Feb 19, 2021

codecov bot commented Feb 19, 2021 • edited Loading

Codecov Report

dyladan commented Feb 19, 2021

dyladan commented Feb 23, 2021 • edited Loading

MSNev commented Feb 23, 2021

dyladan commented Feb 23, 2021 • edited Loading

MSNev commented Feb 23, 2021

dyladan commented Feb 23, 2021

obecny commented Feb 23, 2021

dyladan commented Feb 23, 2021

obecny commented Feb 23, 2021

MSNev commented Feb 23, 2021 • edited Loading

dyladan commented Feb 23, 2021

vmarchaud commented Feb 23, 2021

MSNev commented Feb 23, 2021

Choose a reason for hiding this comment

dyladan commented Feb 23, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dyladan Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

Flarna commented Feb 24, 2021

dyladan commented Feb 24, 2021

dyladan commented Feb 24, 2021

codecov bot commented Feb 19, 2021 •

edited

Loading

dyladan commented Feb 23, 2021 •

edited

Loading

dyladan commented Feb 23, 2021 •

edited

Loading

MSNev commented Feb 23, 2021 •

edited

Loading

dyladan commented Feb 23, 2021 •

edited

Loading

dyladan Feb 24, 2021 •

edited

Loading