Skip to content

feat(node): Remove lru_map dependency #9300

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions packages/deno/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,9 @@
"@sentry/browser": "7.74.1",
"@sentry/core": "7.74.1",
"@sentry/types": "7.74.1",
"@sentry/utils": "7.74.1",
"lru_map": "^0.3.3"
"@sentry/utils": "7.74.1"
},
"devDependencies": {
"@rollup/plugin-commonjs": "^25.0.5",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this this just there for lru_map?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep!

"@rollup/plugin-typescript": "^11.1.5",
"@types/node": "20.8.2",
"rollup-plugin-dts": "^6.1.0"
Expand Down
2 changes: 0 additions & 2 deletions packages/deno/rollup.config.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// @ts-check
import nodeResolve from '@rollup/plugin-node-resolve';
import commonjs from '@rollup/plugin-commonjs';
import sucrase from '@rollup/plugin-sucrase';
import { defineConfig } from 'rollup';

Expand All @@ -21,7 +20,6 @@ export default defineConfig({
nodeResolve({
extensions: ['.mjs', '.js', '.json', '.node', '.ts', '.tsx'],
}),
commonjs(),
sucrase({ transforms: ['typescript'] }),
],
});
3 changes: 1 addition & 2 deletions packages/deno/src/integrations/contextlines.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import type { Event, EventProcessor, Integration, StackFrame } from '@sentry/types';
import { addContextToFrame } from '@sentry/utils';
import { LRUMap } from 'lru_map';
import { addContextToFrame, LRUMap } from '@sentry/utils';

const FILE_CONTENT_CACHE = new LRUMap<string, string | null>(100);
const DEFAULT_LINES_OF_CONTEXT = 7;
Expand Down
1 change: 0 additions & 1 deletion packages/node/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
"@sentry/utils": "7.74.1",
"cookie": "^0.5.0",
"https-proxy-agent": "^5.0.0",
"lru_map": "^0.3.3",
"tslib": "^2.4.1 || ^1.9.3"
},
"devDependencies": {
Expand Down
3 changes: 1 addition & 2 deletions packages/node/src/integrations/contextlines.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import type { Event, EventProcessor, Hub, Integration, StackFrame } from '@sentry/types';
import { addContextToFrame } from '@sentry/utils';
import { addContextToFrame, LRUMap } from '@sentry/utils';
import { readFile } from 'fs';
import { LRUMap } from 'lru_map';

const FILE_CONTENT_CACHE = new LRUMap<string, string[] | null>(100);
const DEFAULT_LINES_OF_CONTEXT = 7;
Expand Down
2 changes: 1 addition & 1 deletion packages/node/src/integrations/http.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@ import {
fill,
generateSentryTraceHeader,
logger,
LRUMap,
stringMatchesSomePattern,
} from '@sentry/utils';
import type * as http from 'http';
import type * as https from 'https';
import { LRUMap } from 'lru_map';

import type { NodeClient } from '../client';
import { NODE_VERSION } from '../nodeVersion';
Expand Down
7 changes: 3 additions & 4 deletions packages/node/src/integrations/localvariables.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
/* eslint-disable max-lines */
import type { Event, EventProcessor, Exception, Hub, Integration, StackFrame, StackParser } from '@sentry/types';
import { logger } from '@sentry/utils';
import { logger, LRUMap } from '@sentry/utils';
import type { Debugger, InspectorNotification, Runtime, Session } from 'inspector';
import { LRUMap } from 'lru_map';

import { NODE_VERSION } from '../nodeVersion';
import type { NodeClientOptions } from '../types';
Expand Down Expand Up @@ -470,8 +469,8 @@ export class LocalVariables implements Integration {
}

// Check if we have local variables for an exception that matches the hash
// delete is identical to get but also removes the entry from the cache
const cachedFrames = this._cachedFrames.delete(hash);
// remove is identical to get but also removes the entry from the cache
const cachedFrames = this._cachedFrames.remove(hash);

if (cachedFrames === undefined) {
return;
Expand Down
2 changes: 1 addition & 1 deletion packages/node/src/integrations/undici/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ import {
dynamicSamplingContextToSentryBaggageHeader,
generateSentryTraceHeader,
getSanitizedUrlString,
LRUMap,
parseUrl,
stringMatchesSomePattern,
} from '@sentry/utils';
import { LRUMap } from 'lru_map';

import type { NodeClient } from '../../client';
import { NODE_VERSION } from '../../nodeVersion';
Expand Down
14 changes: 3 additions & 11 deletions packages/node/test/integrations/localvariables.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { ClientOptions, EventProcessor } from '@sentry/types';
import type { LRUMap } from '@sentry/utils';
import type { Debugger, InspectorNotification } from 'inspector';
import type { LRUMap } from 'lru_map';

import { defaultStackParser } from '../../src';
import type { DebugSession, FrameVariables } from '../../src/integrations/localvariables';
Expand Down Expand Up @@ -178,11 +178,7 @@ describeIf((NODE_VERSION.major || 0) >= 18)('LocalVariables', () => {

expect((localVariables as unknown as LocalVariablesPrivate)._cachedFrames.size).toBe(1);

let frames: FrameVariables[] | undefined;

(localVariables as unknown as LocalVariablesPrivate)._cachedFrames.forEach(f => {
frames = f;
});
const frames: FrameVariables[] = (localVariables as unknown as LocalVariablesPrivate)._cachedFrames.values()[0];

expect(frames).toBeDefined();

Expand Down Expand Up @@ -274,11 +270,7 @@ describeIf((NODE_VERSION.major || 0) >= 18)('LocalVariables', () => {

expect((localVariables as unknown as LocalVariablesPrivate)._cachedFrames.size).toBe(1);

let frames: FrameVariables[] | undefined;

(localVariables as unknown as LocalVariablesPrivate)._cachedFrames.forEach(f => {
frames = f;
});
const frames: FrameVariables[] = (localVariables as unknown as LocalVariablesPrivate)._cachedFrames.values()[0];

expect(frames).toBeDefined();

Expand Down
1 change: 1 addition & 0 deletions packages/utils/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,4 @@ export * from './userIntegrations';
export * from './cache';
export * from './eventbuilder';
export * from './anr';
export * from './lru';
60 changes: 60 additions & 0 deletions packages/utils/src/lru.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/** A simple Least Recently Used map */
export class LRUMap<K, V> {
private readonly _cache: Map<K, V>;

public constructor(private readonly _maxSize: number) {
this._cache = new Map<K, V>();
}

/** Get the current size of the cache */
public get size(): number {
return this._cache.size;
}

/** Get an entry or undefined if it was not in the cache. Re-inserts to update the recently used order */
public get(key: K): V | undefined {
const value = this._cache.get(key);
if (value === undefined) {
return undefined;
}
// Remove and re-insert to update the order
this._cache.delete(key);
this._cache.set(key, value);
return value;
}

/** Insert an entry and evict an older entry if we've reached maxSize */
public set(key: K, value: V): void {
if (this._cache.size >= this._maxSize) {
// keys() returns an iterator in insertion order so keys().next() gives us the oldest key
this._cache.delete(this._cache.keys().next().value);
}
this._cache.set(key, value);
}

/** Remove an entry and return the entry if it was in the cache */
public remove(key: K): V | undefined {
const value = this._cache.get(key);
if (value) {
this._cache.delete(key);
}
return value;
}

/** Clear all entries */
public clear(): void {
this._cache.clear();
}

/** Get all the keys */
public keys(): Array<K> {
return Array.from(this._cache.keys());
}

/** Get all the values */
public values(): Array<V> {
const values: V[] = [];
this._cache.forEach(value => values.push(value));
return values;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this over Array.from(this._cache.values())?

Copy link
Collaborator Author

@timfish timfish Oct 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That triggers a lint:

ES6 methods not allowed: valueseslint(@sentry-internal/sdk/no-unsupported-es6-methods)

This method is currently only used by some tests...

}
}
41 changes: 41 additions & 0 deletions packages/utils/test/lru.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import { LRUMap } from '../src/lru';

describe('LRUMap', () => {
test('evicts older entries when reaching max size', () => {
const map = new LRUMap<string, string>(3);
map.set('a', '1');
map.set('b', '2');
map.set('c', '3');
map.set('d', '4');
map.set('e', '5');

expect(map.keys()).toEqual(['c', 'd', 'e']);
});

test('updates last used when calling get', () => {
const map = new LRUMap<string, string>(3);
map.set('a', '1');
map.set('b', '2');
map.set('c', '3');

map.get('a');

map.set('d', '4');
map.set('e', '5');

expect(map.keys()).toEqual(['a', 'd', 'e']);
});

test('removes and returns entry', () => {
const map = new LRUMap<string, string>(3);
map.set('a', '1');
map.set('b', '2');
map.set('c', '3');
map.set('d', '4');
map.set('e', '5');

expect(map.remove('c')).toEqual('3');

expect(map.keys()).toEqual(['d', 'e']);
});
});
17 changes: 0 additions & 17 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -4800,18 +4800,6 @@
magic-string "^0.25.7"
resolve "^1.17.0"

"@rollup/plugin-commonjs@^25.0.5":
version "25.0.5"
resolved "https://registry.yarnpkg.com/@rollup/plugin-commonjs/-/plugin-commonjs-25.0.5.tgz#0bac8f985a5de151b4b09338847f8c7f20a28a29"
integrity sha512-xY8r/A9oisSeSuLCTfhssyDjo9Vp/eDiRLXkg1MXCcEEgEjPmLU+ZyDB20OOD0NlyDa/8SGbK5uIggF5XTx77w==
dependencies:
"@rollup/pluginutils" "^5.0.1"
commondir "^1.0.1"
estree-walker "^2.0.2"
glob "^8.0.3"
is-reference "1.2.1"
magic-string "^0.27.0"

"@rollup/plugin-json@^4.0.0", "@rollup/plugin-json@^4.1.0":
version "4.1.0"
resolved "https://registry.yarnpkg.com/@rollup/plugin-json/-/plugin-json-4.1.0.tgz#54e09867ae6963c593844d8bd7a9c718294496f3"
Expand Down Expand Up @@ -19997,11 +19985,6 @@ lru-cache@^9.0.0:
resolved "https://registry.yarnpkg.com/lru-cache/-/lru-cache-10.0.0.tgz#b9e2a6a72a129d81ab317202d93c7691df727e61"
integrity sha512-svTf/fzsKHffP42sujkO/Rjs37BCIsQVRCeNYIm9WN8rgT7ffoUnRtZCqU+6BqcSBdv8gwJeTz8knJpgACeQMw==

lru_map@^0.3.3:
version "0.3.3"
resolved "https://registry.yarnpkg.com/lru_map/-/lru_map-0.3.3.tgz#b5c8351b9464cbd750335a79650a0ec0e56118dd"
integrity sha1-tcg1G5Rky9dQM1p5ZQoOwOVhGN0=

lunr@^2.3.8:
version "2.3.9"
resolved "https://registry.yarnpkg.com/lunr/-/lunr-2.3.9.tgz#18b123142832337dd6e964df1a5a7707b25d35e1"
Expand Down