https://github.com/machinefi/w3bstream/pull/395

[juntao]

No description provided.

[juntao]

Hello, I am a code review bot on flows.network. Here are my reviews of changed source code files in this PR.

---

pkg/modules/deploy/instance_context.go

This code seems to be in fairly good shape, but there are some suggestions and potential issues:

1. Error checking missing: In the following code block:

if canBeInit, ok := c.(wasm.ConfigurationWithInit); ok {
	err = canBeInit.Init(ctx)
}

err from the canBeInit.Init(ctx) should be checked before we continue the loop.

if canBeInit, ok := c.(wasm.ConfigurationWithInit); ok {
	err = canBeInit.Init(ctx)
	if err != nil {
		return nil, status.ConfigInitializationFailed.StatusErr().WithDesc(err.Error())
	}
}

2. Import naming: It seems that you renamed the import github.com/machinefi/w3bstream/pkg/depends/conf/id to confid. While this is a minor issue, it might make your code slightly harder to read for an outsider. If possible, try to stick with the original package names to maintain consistency.

3. Comments and documentation: Add comments to the functions and their parameters to explain their purpose and usage. In this particular code, adding comments to the WithInstanceRuntimeContext function will make the code more readable and maintainable for others.

4. Package name: It's not apparent what the deploy package is supposed to do; a more descriptive name for the package or an overview comment at the beginning of the file can make the codebase more readable.

5. TODO item: There's a TODO item in the code:

// TODO: the node's address
ctx = wasm.WithCustomMetrics(ctx, custommetrics.NewCustomMetric("test", strconv.FormatUint(uint64(acc.AccountID), 10), prj.ProjectName.Name))

Make sure to address this and implement the necessary functionality or provide more context regarding the TODO item for future development.

6. Context composition: Using contextx.WithContextCompose to create a new context with all the values is nice, but it might be more efficient and scalable to create a function to handle all the context assignments, especially when more values are added to the context in the future. It can make the code easier to read and maintain.

7. Error wrapping: When returning errors, it is a good practice to wrap them with additional context to make debugging easier in case of an error. You can use fmt.Errorf("some message: %w", err) or a package like github.com/pkg/errors to wrap and add context to your errors.

The following key changes were made in the patch:

1. Added strconv package: The strconv package has been imported which provides functions for converting between string and numeric data types.

+	"strconv"

2. Added custom metrics: A new line has been added to create custom metrics using the wasm.WithCustomMetrics function, which uses accountID and projectName.Name as parameters.

+	// TODO: the node's address
+	ctx = wasm.WithCustomMetrics(ctx, custommetrics.NewCustomMetric("test", strconv.FormatUint(uint64(acc.AccountID), 10), prj.ProjectName.Name))

Essentially, the patch added a new import for the strconv package and added a line to create custom metrics using the accountID and projectName.Name.

pkg/modules/vm/wasmtime/instance_exports.go

1. Import error handling:

Lines 24-33 import several external dependencies but do not have proper error handling. If any of these imported external dependencies are not available, the code will panic during runtime. You should check for errors and return a descriptive error message for better debugging.

Suggestion:

ef.cl, err = wasm.ChainClientFromContext(ctx)
if err != nil {
	return nil, errors.Wrap(err, "failed to get ChainClient from context")
}

// Similarly, for other contexts.

2. The _rand variable is package-level:

The _rand variable is defined at the package level and assigned a value on line 42. The package-level variables are shared across all instances of the package, which may not be desirable for a random number generator. You should consider creating an instance-level random variable inside the ExportFunc struct, and initialize it using a seed in the NewExportFuncs method to ensure that each instance has its own random number generator.

3. Using a single random generator:

The use of a single seeded random number generator can cause issues with predictability and can be an issue in concurrent execution, as it is not thread-safe.

Consider using sync.Mutex to lock and unlock the rand.Read function access or use math/rand.(*Rand).Read function with a lock.

4. Avoid returning error codes as integers:

Functions such as GetData (ef *ExportFuncs) use integer return codes to communicate errors. This is an old C-style pattern that doesn't take advantage of Go's built-in error handling. Consider returning (int32, error) from these functions, and in the case of an error, you can return a proper error message using errors.New() or fmt.Errorf().

5. Inefficient string manipulation:

In the Trace function on Line 162:

str := strings.Trim(strings.Join(strings.Fields(fmt.Sprint(arr)), ", "), "[]")

Using fmt.Sprint, strings.Join and strings.Trim together is inefficient. You can try to replace it with more efficient string manipulation functions or with the use of strings.Builder.

6. Add a context parameter to functions with external dependencies:

For some functions like SetSQLDB, GetSQLDB, SendTX, SendMQTT, and CallContract where you call external functionalities like database access, network calls or external libraries, consider adding context.Context as a parameter in the function signature, which allows you to cancel operations, set timeouts, or pass deadlines.

7. Log functions should not dispatch a job:

The functions like Log on Line 122 dispatches a job even after logging successfully inside the function. Please check if this is expected behavior.

job.Dispatch(ef.ctx, job.NewWasmLogTask(ef.ctx, conflog.Level(logLevel).String(), string(buf)))

Overall, the code seems to be properly formatted, and the logic seems fine. Make sure to address these issues to improve the efficiency, maintainability, and error handling in the code.

This patch introduces the following key changes to the code:

1. A new field metrics is added to the ExportFuncs struct. This field represents custom metrics defined by the type custommetrics.Metrics.

2. In the NewExportFuncs function, ef.metrics is now being initialized by calling wasm.CustomMetricsFromContext(ctx).

3. In the LinkABI function, new metric-related functions are added to the import map: "ws_counter_inc", "ws_counter_add", and "ws_gauge_set". These functions map to ef.StatCounterInc, ef.StatCounterAdd, and ef.StatGaugeSet.

4. Three new functions are implemented: StatCounterInc, StatCounterAdd, and StatGaugeSet. Each function reads a label from the runtime, updates the corresponding metric, and returns a result status code. The StatCounterInc function increments the counter of the specified label, StatCounterAdd adds a value to the counter, and StatGaugeSet sets the value of the specified gauge.

In summary, the patch integrates custom metrics support into the existing code, allowing the use and manipulation of custom metric counters and gauges.

pkg/types/wasm/context.go

The following observations were made on the provided source code:

1. The code seems to be defining a set of context keys and associated helper functions for storing and retrieving various components from a context.Context. These components include SQLStore, KVStore, logger, environment, redis prefix, chain client, runtime resource, runtime event types, MQTT client, and custom metrics.

2. There are no direct issues with the code as such, but there could be some improvements and best practices that can be applied:

   a. Using a separate package for the context keys and helper functions would make it more modular and easily maintainable. The current package seems to be named as wasm which does not provide much information about the purpose of the code since all the code deals with context management.

   b. The context keys (such as CtxSQLStore, CtxKVStore, and others) are defined as exported structs (i.e., starting with an uppercase letter), this makes them accessible outside of the package. However, it's generally recommended to keep the context keys un-exported (i.e., lowercase starting letter) to prevent clashes with other context keys defined in different packages of the application.

   c. Instead of defining separate functions for each component (e.g., WithSQLStore, WithKVStore, and so on), consider defining a generic function that accepts a context key, a component value, and a context as input and returns a new context modified with the value associated with the key. This would reduce code repetition and provide a more consistent interface for managing context values. The same can be applied to getter functions and context extractor functions.

3. Some of the helper functions (e.g., MustSQLStoreFromContext, MustKVStoreFromContext, and so on) use a function from the must package, which is called must.BeTrue(). It is unclear from the provided code what this function does, make sure that it handles error cases appropriately.

In conclusion, there are no direct issues with the code, but it can be refactored to be more modular, maintainable, and follow best practices.

The patch introduces a new context key named CtxCustomMetrics, along with its associated helper functions:

1. WithCustomMetrics: This function takes a context and a custom metrics object as input and returns a new context that associates the custom metrics object with the CtxCustomMetrics key.

2. WithCustomMetricsContext: This function takes a custom metrics object as input and returns a higher-order function that, when invoked with a context, associates the custom metrics object with the CtxCustomMetrics key in the context.

3. CustomMetricsFromContext: This function takes a context as input and tries to extract a custom metrics object associated with the CtxCustomMetrics key. It returns the custom metrics object if it exists along with a boolean flag indicating if the extraction was successful.

4. MustCustomMetricsFromContext: This function takes a context as input and extracts a custom metrics object using CustomMetricsFromContext. If the extraction is successful, the custom metrics object is returned. If not, must.BeTrue() is called, which is assumed to handle the error case appropriately.

In summary, the patch adds support for storing and retrieving custom metrics objects in a context.Context using a new context key (CtxCustomMetrics) and associated helper functions.

pkg/types/wasm/metrics/metrics.go

1. Typo in Gauge metric name:
   In the _customMetricsGauge, the Name field has a typo: "ws_wasm_custom_gague_metrics". This should be corrected to "ws_wasm_custom_gauge_metrics"

2. Commented methods in Gauge interface and gauge struct:
   Either remove the commented methods (Add() and Sub()) in the Gauge interface if they are not required, or uncomment them if these methods should be exposed for the Gauge type.

3. Add synchronization for Load and Store operations in Counter() and Gauge() methods:
   The Load method and the subsequent Store method should be synchronized to avoid concurrent read and write issues. This can be achieved using the LoadOrStore function of sync.Map, which would provide synchronization implicitly.

Refactor the Counter() method:

func (m *metrics) Counter(customLabel string) Counter {
	value, _ := m.counters.LoadOrStore(customLabel, &counter{
		labels:  append(m.preDefLabels[:], customLabel),
		counter: _customMetricsCounter,
	})
	return value.(Counter)
}

And the Gauge() method:

func (m *metrics) Gauge(customLabel string) Gauge {
	value, _ := m.gagues.LoadOrStore(customLabel, &gauge{
		labels: append(m.preDefLabels[:], customLabel),
		gauge:  _customMetricsGauge,
	})
	return value.(Gauge)
}

4. Redundant type parentheses:

The type parentheses can be removed while declaring the structs, as they are not necessary.

For example, change

type (
	metrics struct {

to

type metrics struct {

Apply this change to all structs in the code.

5. Typo in variable names:
   In the 'NewCustomMetric' function and the 'metrics' struct, gagues should be corrected to gauges.

That's all I could find in the provided source code. Keep in mind that some issues may not have been caught due to truncation of the code.

The patch represents the original code without any changes. Here is a summary of the key elements present in the code:

1. The code defines three interfaces: Metrics, Counter, and Gauge. Metrics is for creating Counter and Gauge instances, while Counter and Gauge interfaces define methods to manipulate their respective metric types.
2. The global variables _customMetricsCounter and _customMetricsGauge hold instances of Prometheus CounterVec and GaugeVec respectively, using an array of labels defined in the _labels variable.
3. The init() function registers these global Prometheus metric instances.
4. The "metrics" struct is defined to manage Counters and Gauges using sync.Map for thread-safety.
5. The NewCustomMetric() function creates a new instance of 'metrics' struct with predefined labels such as node, account, and project.
6. The Counter() and Gauge() methods on the 'metrics' struct fetch or create new Counter or Gauge instances using the provided customLabel.
7. Counter and Gauge structs are defined and implemented with methods Inc() and Add() for Counter and Set() for Gauge, which in turn interact with the Prometheus metric instances.

Please note that this summary is based on the original code and does not include the recommended changes from the code review.

cc iotexproject/w3bstream#395

[juntao]

flows summarize

[juntao]

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.

---

This GitHub Pull Request introduces a new custom metrics feature for a WebAssembly (WASM) virtual machine. The source code has been updated in several key areas, such as changes to the ExportFuncs struct, LinkABI function, and the creation of new functions like StatCounterInc, StatCounterAdd, and StatGaugeSet. These are significant changes that have the potential to affect other aspects of the project.

The primary concerns and potential issues include:

1. The Metrics field in the ExportFuncs struct being initialized as nil in NewExportFuncs, which could lead to nil pointer dereferences when calling the new stat functions.
2. Handling of error codes like wasm.ResultStatusCode_Failed by callers of the newly added functions.
3. A new custom metrics feature implementation that retrieves metrics from context, adds a node's address, and updates label names. This could affect other components that depend on these files, so thorough testing is necessary.

Additionally, the following items should be addressed for improved code quality and functionality:

1. Implement the node's address in instance_context.go, as it is currently marked as "TODO."
2. Remove any commented-out code that is not being used.
3. Ensure compatibility and proper handling of new features with other components and dependencies within the project.

Details

Commit 2fd1dca39d2c732cb07b41e5820d45034e8bed45

Summary of key changes:

1. A new package custommetrics is created in pkg/types/wasm/metrics/metrics.go with different metric interfaces, Counter and Gauge, and their respective implementations. It also adds some Prometheus metrics collectors.
2. The ExportFuncs struct in pkg/modules/vm/wasmtime/instance_exports.go has been modified to include an additional field, metrics of type custommetrics.Metrics.
3. A new constructor function NewExportFuncs is added that initializes the new ExportFuncs.
4. The LinkABI function is updated to include mappings for the added stat functions: ws_counter_inc, ws_counter_add, and ws_gauge_set.
5. Three new functions StatCounterInc, StatCounterAdd, and StatGaugeSet have been added to interact with the metrics interfaces.

Potential issues:

1. The Metrics field in the ExportFuncs struct is initialized as nil in NewExportFuncs. This could lead to nil pointer dereferences when calling the added functions, such as StatCounterInc, which will operate on the uninitialized Metrics field. It should be initialized properly before using.
2. The StatCounterInc, StatCounterAdd, and StatGaugeSet functions use wasm.ResultStatusCode_Failed when errors occur. However, it is not clear if callers are correctly handling these error codes.
3. While not directly an issue, commented out code at the end of the metrics.go file should ideally be removed, since it's not being used.

Commit 4f0db4fcc5c650d00f802fba19a9dca2f9449da8

This GitHub patch adds a new custom metric feature for a WebAssembly (WASM) virtual machine. The key changes are in four files:

1. instance_context.go: A new custom metric instance is added and attached to the context. The address of the node is marked as a TODO for future implementation.
2. instance_exports.go: The custom metric is retrieved from the context.
3. context.go: Functions for setting and getting custom metrics from the context are implemented.
4. metrics.go: Labels for the custom metrics are updated, and a new variable _labels is added to store the label names.

Potential problems:

1. The node's address in instance_context.go is marked as "TODO." It needs to be implemented so the custom metrics can be associated with a particular node.
2. Any other dependencies on these files that expect the previous behavior may be affected by these changes. Make sure to test other components of the project to ensure compatibility.

cc iotexproject/w3bstream#395