bug: fix metadata-token-service.ts #135

Zork33 · 2023-07-25T11:36:54Z

close #130

nikolaymatrosov

Не уверен, что схема с lastFetch будет работать.

Zork33 · 2023-08-01T06:51:25Z

Talked to Iam services, found the tokenization rules. Now the code is based on the received expires_in and the rule that the returned token should expire not earlier than in 15 min.

nikolaymatrosov · 2023-08-01T07:13:49Z

src/token-service/metadata-token-service.ts

        }

-        return this.token;
+        return this.token as string;


In the TypeScript code review, it is advised to avoid using the as keyword wherever possible. Relying on as can obscure potential type-related problems in the code, so it's better to find alternative approaches that ensure type safety and clarity throughout the codebase.

In this case "as" is necessary for an optional field to become mandatory. Before this line there is a check that the value is defined. Probably need some kind of hint for ts review.

this.token! is more idiomatic

превел на ! там где это подходит

nikolaymatrosov · 2023-08-01T07:16:41Z

src/token-service/metadata-token-service.ts

+        // deduplicate initialize requests in any async case
+        if (!this.currentInitialize) {
+            this.currentInitialize = this._initialize().finally(() => {
+                delete this.currentInitialize;


In general, prefer assigning null to a class property rather than using the delete operator because it is more straightforward, safer, and more in line with good TypeScript practices.

This is a good rule in Java. In TS, if a variable is described as "private token?: string;" delete is a switch to the initial state. Other variants require additional testing

nikolaymatrosov · 2023-08-01T07:18:53Z

src/token-service/metadata-token-service.ts

            try {
                // eslint-disable-next-line no-await-in-loop
                this.token = await this.fetchToken();
-                break;
+


Adding a delay when implementing retry improves code resilience by mitigating transient failures, preventing server overload, and conserving client resources.

That's the way it used to be - before this PR. If you have any ideas on what delays to add, I can add them. I don't want to add them just like that, because it may increase initialization time

src/token-service/metadata-token-service.ts

nikolaymatrosov · 2023-08-01T07:23:44Z

src/token-service/metadata-token-service.ts

 export class MetadataTokenService implements TokenService {
    private readonly url: string;
    private readonly opts: Options;
    private token?: string;
+    private tokenExpired = 0;
+    private tokenTimeToRefresh = 0;


It is not clear why do we need two different timestamps. Why can't one be derived out another?

There are two key time points for updating a token - when 10% of the lifetime has passed and when there are 15 minutes of lifetime left. To avoid repeating these calculations in different places (which can be a source of errors), it is easier to calculate them once when receiving a token

nikolaymatrosov · 2023-08-01T07:26:22Z

src/token-service/metadata-token-service.ts

-        const res = await axios.get<{ access_token: string }>(this.url, this.opts);
+
+        // deduplicate real fetch token requests in any async case
+        if (!this.currentFetchToken) {


Why don't you invert the if statement and return existing promise early?

It's a more comprehensible code. Otherwise there will be two return statements

nikolaymatrosov · 2023-08-01T07:26:55Z

src/token-service/metadata-token-service.ts

+            try {
+                this.token = await this.fetchToken();
+            } catch {
+                // nothing - use old token


Why there is no error handling?

No need - we can continue to use the old token

nikolaymatrosov · 2023-08-01T07:32:36Z

src/token-service/metadata-token-service.ts

+        const timeLeft = res.data.expires_in * 1000 - TOKEN_MINIMUM_LIFETIME_MARGIN_MS;
+
+        if (timeLeft <= 0) {
+            throw new Error('failed to fetch token: insufficient lifetime');


That is wrong. The token we got is totally usable. Is this error retriable?

This logic is for the hypothetical case if a token with less than 15 min remaining lifetime is returned. Now it is not reproducible - as tokens with 10+ hours of reserve are returned

nikolaymatrosov · 2023-08-01T07:34:30Z

src/token-service/metadata-token-service.ts

-        for (let i = 0; i < 5; i++) {
+        delete this.token;
+
+        for (let i = 0; i < MAX_ATTEMPTS_NUMBER_TO_GET_TOKEN_IN_INITIALIZE; i++) {


Why there are retries only in initialize phase?

Because if we already have a token and it has at least 15 min life left. In case of an error, there is no need to retry, we can continue to use the old token.

nikolaymatrosov · 2023-08-01T07:36:26Z

.eslintrc.json

@@ -50,6 +50,7 @@
    "import/no-extraneous-dependencies": ["error", {
      "devDependencies": true
    }],
-    "import/no-cycle": "off"
+    "import/no-cycle": "off",
+    "linebreak-style": "off"


Turn this back on, please. And fix linebreaks in the IDE settings.

Since I'm developing on Windows, if I don't enable this option, I get an error on every line ) However, any windows git client converts crlf to lf during any git commit

As I said, you should fix it in the IDE settings.

а кто нибуть на практике пробовал так работать? потому что для windows crlf возникает на всех шагах - когда файлы из git приходят, когда редактируешь любым не специально настроенным редактором, генераторы кода тоже с таким переносом пишут. так что просто настройки IDE задачу увы не решают

package.json

src/token-service/metadata-token-service.ts

DavyJohnes · 2023-08-07T13:46:47Z

src/token-service/metadata-token-service.ts

@@ -15,64 +21,93 @@ export class MetadataTokenService implements TokenService {
    private readonly url: string;
    private readonly opts: Options;
    private token?: string;
+    private tokenExpiredAt = 0;
+    private tokenTimeToRefresh = 0;
+    private currentFetchToken?: Promise<string>;


I would suggest to use something like https://github.com/DirtyHairy/async-mutex in order to guarantee single fetch at any point of time.

I looked at this library. It is about mutexes and simophores, which doesn't seem solving my task. I need that one internal query is formed for several parallel queries, and all queries return the same result - promis sharing solves it

esseswann · 2023-08-31T08:15:14Z

Are there any chances this going to be merged soon?

DavyJohnes · 2023-10-09T08:39:02Z

.eslintrc.json

@@ -50,6 +50,7 @@
    "import/no-extraneous-dependencies": ["error", {
      "devDependencies": true
    }],
-    "import/no-cycle": "off"
+    "import/no-cycle": "off",
+    "linebreak-style": "off"


@Zork33 давай, пожалуйста, вернем это правило, с учетом того что Коля выше описал.

это правило ломает разработку под windows - lint начинает выглядеть вот так,

если его возвращать, то хорошо бы придумать как это обойти под виндой. вариант чтобы под виндой были просто lf переносы не встречал. при этом, как я уже говорил, git под виндой сам нормализует crlf -> lf при комите. Есть ли реальная проблема?

DavyJohnes · 2023-10-09T08:41:11Z

package.json

-    "lint": "eslint src config",
+    "test": "cross-env NODE_OPTIONS=\"--max-old-space-size=4096\" jest -c config/jest.ts --passWithNoTests",
+    "test:coverage": "jest -c config/jest.coverage.ts --passWithNoTests",
+    "lint": "eslint src config --fix",


Не стоит, эта же команда и в CI выполняется, не очень хорошо что там после выполнения команды останутся изменения.

Понял. Согласен!

DavyJohnes · 2023-10-09T08:41:25Z

package.json

@@ -66,8 +66,9 @@
    "typescript": "^4.5.4"
  },
  "scripts": {
-    "test": "cross-env NODE_OPTIONS=\"--max-old-space-size=4096\" jest -c config/jest.ts --passWithNoTests '.*\\.test\\.ts$'",
-    "lint": "eslint src config",
+    "test": "cross-env NODE_OPTIONS=\"--max-old-space-size=4096\" jest -c config/jest.ts --passWithNoTests",


--passWithNoTests уже можно смело убирать

Похоже убирать не стоит - без нее тесты сыпятся на файле src/generated/yandex/cloud/loadtesting/agent/v1/test.ts Который понятно тестом не является, но под маску тестовых файлов подходит

Проверил и с этой опцией сыпятся. Так что добавил игнор папки src/generated

DavyJohnes · 2023-10-09T08:41:39Z

package.json

-    "test": "cross-env NODE_OPTIONS=\"--max-old-space-size=4096\" jest -c config/jest.ts --passWithNoTests '.*\\.test\\.ts$'",
-    "lint": "eslint src config",
+    "test": "cross-env NODE_OPTIONS=\"--max-old-space-size=4096\" jest -c config/jest.ts --passWithNoTests",
+    "test:coverage": "jest -c config/jest.coverage.ts --passWithNoTests",


А почему cross-env NODE_OPTIONS=\"--max-old-space-size=4096\" тут не нужно?

верну. убрал для теста и забыл

DavyJohnes · 2023-10-09T08:44:46Z

src/__tests__/metadata-token-service/010.constructors.ts

+                await metadataTokenService.dispose();
+
+                expect(testLoggerFn.mock.calls)
+                    .toEqual([


Тестировать логгирование компонента - это уже чересчур, как по мне :)

)) ну на самом деле это вроде как know how. то есть через проверку логов, фиксируется что произошло то что и ожидалась. в частности через проверку debug выдачи проверяется что параметры конструктора добрались куда должны были

DavyJohnes · 2023-10-09T09:30:16Z

src/token-service/metadata-token-service.ts

+
+/* istanbul ignore next */
+// eslint-disable-next-line @typescript-eslint/no-namespace,import/export
+export namespace MetadataTokenService {


А зачем неймспейс? Да и в целом, у тебя есть модуль с константами для metadata-token-service, кажется этим константам там и место?

Неймспейс появился для интерфейсов. Просто если объявить интерфейс Logger глобальным, то по правилам ts его может расширить интерфейс Logger из другого кода, если они вдруг встретятся. Вроде namespace для этого как раз и нужен

А константы туда попали по инерции. Перенесу их в consts

DavyJohnes · 2023-10-09T09:33:30Z

src/token-service/metadata-token-service.ts

+
+        delete this.token;
+
+        for (let i = 1; ; i++) {


А почему у нас ретраи на фетч токена есть только в initialize()? Кажется что в getToken() тоже хочется ретраить?

Идея в том и есть комментарий в коде - что если есть активный токен, то ошибка get token не требует моментальных ретраев, и продолжает использоваться имеющийся токен. При этом факт ошибки учитывается при вычислении времени когда повторно сходить за токеном - это ретраи

DavyJohnes · 2023-10-09T09:34:59Z

src/token-service/metadata-token-service.ts

+            // eslint-disable-next-line no-bitwise
+            const slotsCount = 1 << Math.min(i, INITIALIZE_BACKOFF_CEILING);
+            const maxDuration = slotsCount * INITIALIZE_BACKOFF_SLOT_DURATION;
+            const duration = Math.trunc(maxDuration * (1 - Math.random() * INITIALIZE_BACKOFF_UNCERTAIN_RATIO));


Зачем мы сами имплементируем ретраи, если можно просто заюзать axios-retry? Кажется в нем есть уже все нужные опции.

да - axiios-retry можно использовать в initialize(). для getToken() он не подходит - так повторы не в цикле. предлагаю оставить как есть - уже протестировано и минус одна дополнительная зависимость

DavyJohnes · 2023-10-09T09:35:58Z

src/utils/simple-logger.ts

@@ -0,0 +1,149 @@
+const DEFAULT_ENV_KEY = 'LOG_LEVEL';


А зачем мы свой логгер имплементим? Чем любой из доступных в npm не подходит?

Ну я действовал по аналогии с ydb-sdk, чтобы не создавать лишние зависимости, проще иметь свой простой логер. Те логеры которые я знаю в npm довольно много зависимостей тянут и вариантов много. А так есть логер для простых тестов - консоль всегда есть. Кстати, для тестов в облаке я еще вот такой варант написал (https://github.com/Zork33/yandex-cloud-simple-logger) - может знаешь какую нить другую реализацию простого yc-логирования

DavyJohnes · 2023-10-09T09:40:00Z

src/utils/tests/fake-timers-fixture.ts

+        // this additional logic ensures that by the end of advanceTimer(), all handlers including asynchronous
+        // handlers on timers will be completed
+        // eslint-disable-next-line @typescript-eslint/ban-types
+        await Promise.all(this.timeouts);


А вот это разве не тоже самое? https://jestjs.io/docs/jest-object#jestrunalltimersasync

Судя по описанию это то что нужно. Правда появилось недавно и я пока не понял, что именно нужно обновить. Поставил jest 29.5.5 но при этом получаю такую ошибку

Там есть подсказка
This function is not available when using legacy fake timers implementation.

Ага - я ее видел. Но беглый поиск не дал ответа, что именно надо обновить. Потому вернусь к этом вопросу позже-

nikolaymatrosov requested changes Jul 25, 2023

View reviewed changes

Zork33 force-pushed the fix-metadata-token-service branch from f9df471 to 7256bcf Compare August 1, 2023 06:44

nikolaymatrosov requested changes Aug 1, 2023

View reviewed changes

bug: fix metadata-token-service.ts

b47248b

Zork33 force-pushed the fix-metadata-token-service branch from 7256bcf to b47248b Compare August 1, 2023 09:41

DavyJohnes reviewed Aug 7, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

src/token-service/metadata-token-service.ts Outdated Show resolved Hide resolved

DavyJohnes reviewed Aug 7, 2023

View reviewed changes

bug: rewrite metadata-token-service.ts

1492b04

Zork33 requested a review from nikolaymatrosov September 28, 2023 09:29

Zork33 force-pushed the fix-metadata-token-service branch from e10bb4e to 1492b04 Compare October 1, 2023 09:51

DavyJohnes requested changes Oct 9, 2023

View reviewed changes

fix: after review

c59ff46

Zork33 force-pushed the fix-metadata-token-service branch from dfd0fe5 to c59ff46 Compare October 9, 2023 22:44

Zork33 requested a review from DavyJohnes October 10, 2023 05:33

Alexey Zorkaltsev added 2 commits October 10, 2023 10:52

chore: fix tests in github

a6e76a5

chore: fix simple-logger defaults

213cbbb

Zork33 force-pushed the fix-metadata-token-service branch from e3d1694 to 213cbbb Compare October 10, 2023 08:01

Alexey Zorkaltsev added 2 commits October 10, 2023 11:27

chore: exoport TokenService interface

b97b75b

Merge branch 'master' into fix-metadata-token-service

1386241

bug: fix metadata-token-service.ts #135

Are you sure you want to change the base?

bug: fix metadata-token-service.ts #135

Conversation

Zork33 commented Jul 25, 2023

nikolaymatrosov left a comment

Choose a reason for hiding this comment

Zork33 commented Aug 1, 2023

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

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

nikolaymatrosov Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Zork33 Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

DavyJohnes Aug 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esseswann commented Aug 31, 2023

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

Zork33 Oct 10, 2023 • 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

Zork33 Oct 9, 2023 • 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

Zork33 Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

nikolaymatrosov Aug 1, 2023 •

edited

Loading

Zork33 Oct 9, 2023 •

edited

Loading

DavyJohnes Aug 7, 2023 •

edited

Loading

Zork33 Oct 10, 2023 •

edited

Loading

Zork33 Oct 9, 2023 •

edited

Loading

Zork33 Oct 9, 2023 •

edited

Loading