Skip to content
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

Go Client Library v3 #205

Open
russorat opened this issue Oct 8, 2020 · 10 comments
Open

Go Client Library v3 #205

russorat opened this issue Oct 8, 2020 · 10 comments
Assignees
Labels
Milestone

Comments

@russorat
Copy link

russorat commented Oct 8, 2020

Proposal: new major version of InfluxDB Go client

Introduction

For a Go developer, the Go API client is likely to be the first concrete
interaction they'll have with the Influx code base. Thus it's an important API
and one where user experience is important. This proposal considers the current
API from that point of view, and proposes a new major version of the module
with a view to improving it.

Problem

Some aspects of a language interface to a network API package that are important:

idiomatic: the package should be "obvious" to people that are used to the language.
upgradable: it should be possible to add new features to the API without breaking backward compatibility.
easy to use correctly: using the package in the most obvious way should also be the correct way to use it.
simple: the package should be as small and simple as possible given the constraints of the API.
consistent: the package should be consistent with itself and with other parts of the Influx code base when possible.

The current Go API has some shortcomings in the above respects:

Options: could use a simple struct with fields rather than setters and getters. The current Options API increases the package surface area considerably. It supports a "fluent" API which isn't very idiomatic in Go compared to a struct literal.
Use of interfaces: all the client API entry points are defined in interface types. Since adding a method to an interface is a breaking change, this means that it's not possible to add new entry points without breaking the API.
Writing points to InfluxDB: the current WriteAPI is hard to use correctly and the current implementation could use improvement. See below for more discussion of this.
The API defines its own Point type but also exposes types from the line-protocol package, mixing two not-necessarily-consistent line-protocol implementations.
There are many exposed packages in the API module, making the whole API surface area hard to see.
Go has a strong convention that the last element of the import path is the package name, but that's not the case ("influx-client-go" vs "influxdb2").
Time durations are often specified as integers, making it unclear what the units are. In Go, it's idiomatic to express durations as time.Duration.
The Find* endpoints tend to return a pointer-to-slice rather than the slice itself, which is probably not necessary.

Write API issues

Go has a strong tradition of checking for errors, but WriteAPI makes it somewhat hard to check correctly for errors.
It provides access to an error channel that can be read to find out any errors that occurred when points were written,
but to use it correctly requires starting a goroutine and then synchronizing with that to obtain the errors.
Consider the Write-API-Errors example from the documentation. This example is subtly wrong: even though the writer
is flushed, there is no guarantee that all the errors have been printed by the time the program exits.
The error-reading goroutine might have read from the channel but have context-switched
before it gets around to printing the error.

The implementation uses goroutines to do the writes asynchronously under the hood,
but from inspection of the code, it appears that there are some shortcomings with the implementation:

  • WriteAPI.Flush should wait until all requests have been written, but it just polls waiting for
    a channel to become empty
    , and if there's a constant stream of points being written,
    that might never happen, so Flush could block indefinitely.

  • there appears to be no back-pressure: if there is some rate-limiting being applied by the server,
    the writer will be able to continue sending points until the buffer overflows, discarding
    the extra points.

Proposed Solution

I propose that almost all of the Go API gets pulled into a single package that exports only concrete types.
There are existing Go modules that use this approach even for large APIs, and it seems to work well
despite the overall size. The go-github package is one such example. Note that that package
still uses separate types for different aspects of the API, but they're all concrete types, which means
that it's possible to extend without breaking backward compatibility, and that the methods are
all visible at a glance in the doc page.

I propose that an external package should be used for all the line-protocol encoding and decoding, but
I don't believe that the current line-protocol module is sufficient for this - I will create another proposal for an update to that.

I propose that the point-writing API is simplified so that no error channel is required and errors
can be checked in the usual Go way.

I propose that the package be renamed and given an import path that matches the name. We could use
this to avoid making the client "v3".

To make it easier for clients to test against the API, I propose that a "fake" implementation of the
HTTP API be provided so that clients can be tested in a more end-to-end fashion using net/http/httptest. It's possible
that this could largely be generated automatically from the OpenAPI specification.

Filtering

The current API has a plethora of methods for all the various FindX variants.
This proposal uses a generic Filter type. This has advantages and disadvantages.
The main advantage is that the API is smaller and more regular. This is hopefully
not outweighed by the fact that this papers over some irregularity in the API - not
all API endpoints implement each kind of query. Perhaps this could be remedied
in the future by adding some of the missing functionality to the API.

Detailed Solution

// Package influxclient interacts with the InfluxDB network API.
//
// Various parts of the API are divided into sections by entity kind (for example organization,
// label, authorization), each implemented
// by a different Go type. The CRUD operations on each type follow the same pattern,
// defined by this generic interface:
//
//	type CRUD[Entity any] interface {
//		Find(ctx context.Context, filter *Filter) ([]Entity, error)
//		FindOne(ctx context.Context, filter *Filter) (Entity, error)
//		Create(ctx context.Context, entity Entity) (Entity, error)
//		Update(ctx context.Context, entity Entity) (Entity, error)
//		Delete(ctx context.Context, id string) error
//	}
//
// The Filter can be used to select the entities returned by the Find operations.
// Not all filter kinds are supported by all endpoints: see the individual API
// documentation for details.
//
// Note that Filter also provides support for paging (using WithOffset
// and WithLimit) - this is only useful with filters that might return more
// than one entity.
//
// For example, to a maximum of 10 buckets that have find all buckets that are
// associated with the organization "example.com" starting 40 entries into
// the results:
//
// 	client.BucketsAPI().Find(ctx context.Context, &influxapi.Filter{
//		OrgName: "example.com",
//		Limit: 10,
//		Offset: 40,
//	})


package influxclient

// TODO define errors. what errors are defined by the API? eg. ErrNotFound etc.

const DefaultBatchSize = 2000

// Params holds the parameters for creating a new client.
// The only mandatory fields are ServerURL and AuthToken.
type Params struct {
	// ServerURL holds the URL of the InfluxDB server to connect to.
	// This must be non-empty.
	ServerURL string

	// AuthToken holds the authorization token for the API.
	// This can be obtained through the GUI web browser interface.
	AuthToken string

	// DefaultTags specifies a set of tags that will be added to each written
	// point. Tags specified on points override these.
	DefaultTags map[string]string

	// HTTPClient is used to make API requests.
	//
	// This can be used to specify a custom TLS configuration
	// (TLSClientConfig), a custom request timeout (Timeout),
	// or other customization as required.
	//
	// It HTTPClient is nil, http.DefaultClient will be used.
	HTTPClient *http.Client

	// BatchSize holds the default batch size
	// used by PointWriter. If it's zero, DefaultBatchSize will
	// be used. Note that this can be overridden with PointWriter.SetBatchSize.
	BatchSize int

	// FlushInterval holds the default flush interval used by PointWriter.
	// If it's zero, points must be flushed manually.
	// Note that this can be overridden with PointWriter.SetFlushInterval.
	FlushInterval time.Duration
}

// Client implements an InfluxDB client.
type Client struct{
	// unexported fields
}

// WritePoints writes all the given points to the server with the
// given organization id into the given bucket.
// The points are written synchronously. For a higher throughput
// API that buffers individual points and writes them asynchronously,
// use the PointWriter method.
func (c *Client) WritePoints(org, bucket string, points []*influxdata.Point) error

// PointWriter returns a PointWriter value that support fast asynchronous
// writing of points to Influx. All the points are written with the given organization
// id into the given bucket.
//
// The returned PointWriter must be closed after use to release resources
// and flush any buffered points.
func (c *Client) PointWriter(org, bucket string) *PointWriter

// DeletePoints deletes points from the given bucket with the given ID within the organization with
// the given organization ID that have timestamps between the two times provided.
//
// The predicate holds conditions for selecting the data for deletion.
// For example:
// 		tag1="value1" and (tag2="value2" and tag3!="value3")
// When predicate is empty, all points within the given time range will be deleted.
// See https://v2.docs.influxdata.com/v2.0/reference/syntax/delete-predicate/
// for more info about predicate syntax.
func (c *Client) DeletePoints(ctx context.Context, orgID, bucketID string, start, stop time.Time, predicate string) error

// Query sends the given flux query on the given organization ID.
// The result must be closed after use.
func (c *Client) Query(ctx context.Context, org, query string) (*QueryResult, error)

// Ready checks that the server is ready, and reports the duration the instance
// has been up if so. It does not validate authentication parameters.
// See https://docs.influxdata.com/influxdb/v2.0/api/#operation/GetReady.
func (c *Client) Ready() (time.Duration, error)

// Health returns an InfluxDB server health check result. Read the HealthCheck.Status field to get server status.
// Health doesn't validate authentication params.
func (c *Client) Health(ctx context.Context) (*HealthCheck, error)

// Authorization returns a value that can be used to interact with the
// authorization-related parts of the InfluxDB API.
func (c *Client) AuthorizationAPI() *AuthorizationAPI

// BucketAPI returns a value that can be used to interact with the
// bucket-related parts of the InfluxDB API.
func (c *Client) BucketAPI() *BucketAPI

// LabelsAPI returns Labels API client
func (c *Client) LabelsAPI() api.LabelsAPI

// OrganizationAPI returns
func (c *Client) OrganizationAPI() *OrganizationAPI

// UsersAPI returns Users API client.
func (c *Client) UsersAPI() api.UsersAPI

// AuthorizationAPI holds methods related to authorization, as found under
// the /authorizations endpoint.
type AuthorizationAPI struct {
	// unexported fields.
}

// Find returns all Authorization records that satisfy the given filter.
func (a *AuthorizationAPI) Find(ctx context.Context, filter *Filter) ([]*Authorization, error)

// FindOne returns one authorization record that satisfies the given filter.
func (a *AuthorizationAPI) FindOne(ctx context.Context, filter *Filter) (*Authorization, error)

// Create creates an authorization record. The
func (a *AuthorizationAPI) Create(ctx context.Context, auth *Authorization) error

func (a *AuthorizationAPI) SetStatus(ctx context.Context, id authID, status AuthorizationStatus) error

func (a *DeleteAuthorization) Delete(ctx context.Context, id authID) error

type BucketAPI struct {
	// unexported fields
}

// Find returns all buckets matching the given filter.
func (a *BucketAPI) Find(ctx context.Context, filter *Filter) ([]*Bucket, error)

// FindOne returns one bucket that matches the given filter.
func (a *BucketAPI) FindOne(ctx context.Context, filter *Filter) ([]*Bucket, error)

// Create creates a bucket. Zero-valued fields in b will be filled in with defaults.
//
// Full information about the bucket is returned.
func (a *BucketAPI) Create(ctx context.Context, b *Bucket) (*Bucket, error)

// Update updates information about a bucket.
// The b.ID and b.OrgID fields must be specified.
func (a *BucketAPI) Update(ctx context.Context, b *Bucket) (*Bucket, error)

// Delete deletes the bucket with the given ID.
func (a *BucketAPI) Delete(ctx context.Context, bucketID string) error

// LabelAPI holds methods pertaining to label management.
type LabelAPI struct {
	// unexported fields
}

// Find returns all labels matching the given filter.
func (a *LabelAPI) Find(ctx context.Context, filter *Filter) ([]*Label, error)

// FindOne returns one label that matches the given filter.
func (a *LabelAPI) FindOne(ctx context.Context, filter *Filter) ([]*Label, error)

// Create creates a new label with the given information.
// The label.Name field must be non-empty.
// The returned Label holds the ID of the new label.
func (a *LabelAPI) Create(label *Label) (*Label, error)

// Update updates the label's name and properties.
// The label.ID and label.OrgID fields must be set.
// If the name is empty, it won't be changed. If a property isn't mentioned, it won't be changed.
// A property can be removed by using an empty value for that property.
//
// Update returns the fully updated label.
// TODO would this be better if it took the id and orgID as separate args?
func (a *LabelAPI) Update(ctx context.Context, label *domain.Label) (*domain.Label, error)

// Delete deletes the label with the given ID.
func (a *LabelAPI) Delete(ctx context.Context, labelID string) error

// Find returns all organizations matching the given filter.
// Supported filters:
//	name, ID, userID
func (a *OrganizationAPI) Find(ctx context.Context, filter *Filter) ([]*Organization, error)

// FindOne returns one organization matching the given filter.
// Supported filters:
//	name
//	ID
//	userID
func (a *OrganizationAPI) FindOne(ctx context.Context, filter *Filter) (*Organization, error)

// Create creates a new organization. The returned Organization holds the new ID.
func (a  *OrganizationAPI) Create(ctx context.Context, org *Organization) (*Organization, error)

// Update updates information about the organization. The org.ID field must hold the ID
// of the organization to be changed.
func (a *OrganizationAPI) Update(ctx context.Context, org *Organization) (*Organization, error)

// Delete deletes the organization with the given ID.
func (a *OrganizationAPI) Delete(ctx context.Context, orgID string) error

// Members returns all members of the organization with the given ID.
func (a *OrganizationAPI) Members(ctx context.Context, orgID string) ([]ResourceMember, error)

// AddMember adds the user with the given ID to the organization with the given ID.
func (a *OrganizationAPI) AddMember(ctx context.Context, orgID, userID string) error

// AddMember removes the user with the given ID from the organization with the given ID.
func (a *OrganizationAPI) RemoveMember(ctx context.Context, orgID, userID string) error

// Owners returns all the owners of the organization with the given id.
func (a *OrganizationAPI) Owners(ctx context.Context, orgID string) ([]ResourceOwner, error)

// AddOwner adds an owner with the given userID to the organization with the given id.
func (a *OrganizationAPI) AddOwner(ctx context.Context, orgID, userID string) error

// Remove removes the user with the given userID from the organization with the given id.
func (a *OrganizationAPI) RemoveOwner(ctx context.Context, orgID, userID string) error

type UsersAPI struct {
	// unexported fields
}

// Find returns all users matching the given filter.
// Supported filters:
//	userID
//	id (same as userID)
//	username
//	name (same as username)
func (a *UsersAPI) Find(ctx context.Context, filter *Filter) ([]*User, error)

// Find returns one user that matches the given filter.
func (a *UsersAPI) FindOne(ctx context.Context, filter *Filter) ([]*User, error)

// Create creates a user. TODO specify which fields are required.
func (a *UsersAPI) Create(ctx context.Context, user *User) (*User, error)

// Update updates a user. The user.ID field must be specified.
// The complete user information is returned.
func (a *UsersAPI) Update(ctx context.Context, user *User) (*User, error)

// SetPassword sets the password for the user with the given ID.
func (a *UsersAPI) SetPassword(ctx context.Context, userID, password string) error

// SetMyPassword sets the password associated with the current user.
// The oldPassword parameter must match the previously set password
// for the user.
func (a *UsersAPI) SetMyPassword(ctx context.Context, oldPassword, newPassword string) error

// Delete deletes the user with the given ID.
func (a *UsersAPI) Delete(ctx context.Context, userID string) error

// Filter specifies a filter that chooses only certain results from
// an API Find operation. The zero value of a filter (or a nil *Filter)
// selects everything with the default limit on result count.
// 
// Not all Find endpoints support all filter kinds (see the relevant endpoints
// for details).
type Filter struct {
	// Limit specifies that the search results should be limited
	// to n results. If n is greater than the maximum allowed by the
	// API, multiple calls will be made to accumulate the desired
	// count.
	//
	// As a special case, if n is -1, there will be no limit
	// and all results will be accumulated into the same slice.
	//
	// When Limit isn't used, the limit will be the default limit
	// used by the API.
	Limit int
	
	// Offset specifies that the search results should start at
	// the given index.
	Offset int
	
	// UserName selects items associated with
	// the user with the given name.
	UserName string

	// UserID selects items associated with the user
	// with the given ID.
	UserID string

	// OrganizationName selects items associated with the
	// organization with the given name.
	OrganizationName string

	// OrganizationID selects items associated with the
	// organization with the given ID.
	OrganizationID string
}

type QueryResult struct {
	// unexported fields.
}

// NextTable advances to the next table in the result.
// Any remaining data in the current table is discarded.
//
// When there are no more tables, it returns false.
func (r *QueryResult) NextTable() bool

// NextRow advances to the next row in the current table.
// When there are no more rows in the current table, it
// returns false.
func (r *QueryResult) NextRow() bool

// Columns returns information on the columns in the current
// table. It returns nil if there is no current table (for example
// before NextTable has been called, or after NextTable returns false).
func (r *QueryResult) Columns() []TableColumn

// Err returns any error encountered. This should be called after NextTable
// returns false to check that all the results were correctly received.
func (r *QueryResult) Err() error

// Values returns the values in the current row.
// It returns nil if there is no current row.
// All rows in a table have the same number of values.
// The caller should not use the slice after NextRow
// has been called again, because it's re-used.
func (r *QueryResult) Values() []interface{}

// Decode decodes the current row into x, which should be
// a pointer to a struct. Columns in the row are decoded into
// appropriate fields in the struct, using the tag conventions
// described by encoding/json to determine how to map
// column names to struct fields.
func (r *QueryResult) Decode(x interface{}) error

// PointWriter implements a batching point writer.
type PointWriter struct {
	// unexported fields
}

// SetBatchSize sets the batch size associated with the writer, which
// determines the maximum number of points that will be written
// at any one point.
//
// This method must be called before the first call to WritePoint.
func (w *PointWriter) SetBatchSize(n int)

// SetFlushInterval sets the interval after which points will be
// flushed even when the buffer isn't full.
//
// This method must be called before the first call to WritePoint.
func (w *PointWriter) SetFlushInterval(time.Duration)

// WritePoint writes the given point to the API. As points are buffered,
// the error does not necessarily refer to the current point.
func (w *PointWriter) WritePoint(p *influxdata.Point) error

// Flush flushes all buffered points to the server.
func (w *PointWriter) Flush() error

// Close flushes all buffered points and closes the PointWriter.
// This must be called when the PointWriter is finished with.
//
// It's OK to call Close more than once - all calls will return the same error
// value.
func (w *PointWriter) Close() error

Omissions

The following current API features were deliberately omitted from the above design:

  • HTTP client options. All the current options and more are easily available by configuring an HTTP Client
    value directly, and this is arguably more idiomatic because this applies to any HTTP-based client API.
  • Client.Setup. This method is only there to set up an account initially, something which
    isn't really in the usual use case for using the Go API client. For that use case, using the net/http
    package directly seems like it would be appropriate. This means that the NewClient contract can
    be simplified so that it always requires a token, making API usage a little less error-prone.
  • UsersAPI.SignIn and SignOut: if this functionality deemed important, then it seems like it would
    be better off living in the connection parameters rather than in the users API.
  • QueryRaw. The capability to define the returned "dialect" of the CSV doesn't seem to be
    hugely useful, as all the available annotations are provided when using the default query, and
    changing the CSV separator seems of marginal utility. For performance reasons, a client
    might wish to avoid the overhead of parsing CSV and just read the raw response body, but
    this is something that's not hard to do by issuing the request directly.
  • Descending. The current API defines PagingWithDescending with respect to the result-paging
    API, but this doesn't appear to be used or available.
  • CreateAuthorizationWithOrgID. This is less general than providing the whole Organization
    struct and not a whole lot easier to use.
  • UpdateAuthorizationStatusWithID vs UpdateAuthorizationStatus: the latter is just minor syntax
    sugar over the former. Better to stick with one entry point here. Same applies to a bunch of other
    similar methods.
  • Client.Options, Client.ServerURL, Client.HTTPService. These methods seem to be of marginal
    utility.
  • sub-packages: the current module exports 7 externally visible packages. This proposal proposes
    just one, or perhaps two (it might be necessary to generate some of the domain types inside
    a separate package). This should result in a module that's substantially more accessible, laying
    out the whole API in one place.

Implementation Plan

This package API is substantially different from the previous API, but most of the changes are fairly
cosmetic, with the exception of the Writer API code which will require a complete rewrite.

Asynchronous point writing

Currently the Writer API uses several goroutines, is somewhat hard to understand
and arguably not entirely correct. It should be possible to implement the new API using
just one extra goroutine (to invoke any current call), with appropriate backpressure
when the API is rate-limited.

API code generation

The code generated from the OpenAPI spec is arguably not ideal. It might be worth
investigating to see if another generator is available that generates somewhat
more compact and idiomatic code.

Plan of Action

  • Let's start with a new feature branch and a README outlining the status and a skeleton of the API with no implementation. Maybe the README has a list of endpoints and a status (implemented, tested, etc).
  • PRs would be opened for adding implementations for each function in small chunks
  • Leave asynchronous write and line protocol library for last
@russorat russorat added the Epic label Oct 8, 2020
@vlastahajek
Copy link
Contributor

@russorat, there are a lot of details to discuss here, but let's start with the most important.

Let me remind you here, that the original request to this client was to provide a similar API experience with other clients, like Java or C#.

This proposal outlines a totally new client, with a new API and a new package name.

Also, as there was a major release of the current client a few months ago, doing a new major release with a completely new API will harm current users.

This leads to the idea if we wouldn' be better off with a new client in a new repo?

@vlastahajek
Copy link
Contributor

The Write API issues paragraph describes some issues that a reviewer has with
specific parts of the current code. Could be they submitted as individual issues
so that they can be discussed and fixed if necessary?

@aldas
Copy link

aldas commented Oct 24, 2020

I just started to port our project to client v2 and it feels so different from previous version.

At first I was baffled that options fields are all private and then noticed setter methods - fluent api does not feel idiomatic go. Can you imagine being long time Java programmer and starting to look at project written in 'C'like java. All iterations over collections are for (int i = 0; i < 5; i++) style instead of for-each loops or stream api these days - this is how it feels at start - little bit awkward.

As we use different precision for writes (some client application deployments use 1s precision for data and some 1ns) as I saw that precision is now time.duration. What happens when I provide 20s by accident - currently defaults to ns precision for write.
As precision can be different for every write(line) there seems to be no way anymore to set precision for single write. Only way to define precision is to create new client.

Is epoch query parameter now removed from library API (https://docs.influxdata.com/influxdb/v2.0/reference/api/influxdb-1x/query/#epoch)? Now way even to use QueryAPI.Query with custom dialect with DateTimeFormat. We were clever to use it to truncate time form ns to s

@aldas
Copy link

aldas commented Oct 25, 2020

just reiterating what @russorat wrote

Use of interfaces: all the client API entry points are defined in interface types. Since adding a method to an interface is a breaking change, this means that it's not possible to add new entry points without breaking the API.

This version of library suffers same problems as previous version - instead of returning (pointer to) structs methods return interfaces.

This makes:

  • (library user perspective) testing harder because to mock implementation I need to satisfy whole interface even if I use only one method out of it.
  • (maintainer perspective) so you guys/gals can not add new methods without major version change (as per semantic versioning) because this would change interface signature and break library users code.

Previous version at least had TCP/UDP client implementations to 'justify' returning interface but this time only 1 struct is implementing WriteAPIBlocking for example. This 'polymorphic' behavior could have been solved by composition in previous version.

I think there is a Go proverb accept interfaces, return structs and Go is a little bit different than c#/java with its interfaces

p.s. what if I need to add my own 'HTTP headers' for writes? There seems to be no way to access request.

p.s.s. is internal/write/service.HandleWrite even goroutine safe? Lets assume that I'm using WriteAPIBlocking and have instance in my service and use it to write from multiple goroutines - if I'm not wrong then w.retryQueue can have race condition.

if !w.retryQueue.isEmpty() {

@vlastahajek
Copy link
Contributor

@aldas, thank you for your detailed feedback. To your issues:

As we use different precision for writes...

Can share more about your use case? e.g. how many writes of those with s and with ns precision per sec (or per min)
Effective writing (and batching) requires all points with the same precision. It would require keeping a buffer per precision and this could lead to delayed writing of some points. But this may not be an issue.
It would definitely make sense to add this to point by point writing, i.e. WriteAPIBlocking now.

Is epoch query parameter now removed from library API (https://docs.influxdata.com/influxdb/v2.0/reference/api/influxdb-1x/query/#epoch)? No way even to use QueryAPI.Query with custom dialect with DateTimeFormat. We were clever to use it to truncate time form ns to s

The v1 InluxQL query endpoint is not supported by this library. V2 Flux query endpoint currently allows only RFC3339 datetime format.

what if I need to add my own 'HTTP headers' for writes? There seems to be no way to access request.

As you found out, request editing is not supported yet. This could add later,

is internal/write/service.HandleWrite even goroutine safe? Lets assume that I'm using WriteAPIBlocking and have instance in my service and use it to write from multiple goroutines - if I'm not wrong then w.retryQueue can have race condition.

write.Service is not supposed to be synchronized. Synchronizing should be done in the layer above. WriteAPIBlocking fails in this, thanks for pointing this out.

@aldas
Copy link

aldas commented Oct 29, 2020

Can share more about your use case? e.g. how many writes of those with s and with ns precision per sec (or per min)

We have data logging application that logs ship data from different sources. Usually it is 3-6 PLCs (programmable logic controllers) with 100 to 2000 fields. Data from PLCs is stored at 1s precision (1hz is requirement). And then there is data from gyroscope which is logged at 50hz rate (50 request per second) and this is stored with ns precision. Little bit offtopic: we truncate time actually to second and millisecond just to get it out of Influx in one timeframe for latest.

we have our own - lets say async pipeline where input sends data to circular queue. There is background process to periodically (1s intervals) take data from queue and write it into Influx in batches. So if Influx slows down eventually we start to drop/overwrite data in queue.

write.Service is not supposed to be synchronized

This is one thing that bothers me a little - things are named as client / api and they have this running miniature microverse in them. I would not expect that from client. In my understanding it should just serialize my data and send it as a HTTP request.

This is little bit too complicated for serialize my slice of structs and send it with http request.

I mean - client is caching actually APIs instances. so for same org+bucket I actually get same API but same time I should not use this API concurrently from multiple goroutines.

why cant writing be as dead simple as

type Influx struct {
	apiURL string
	// org, bucket etc.
	// no references etc - struct that can considered as value 'object'
}

func main() {
	influx := Influx{apiURL: ""} // is some fancy New(api, org, bucket, ...opts) constructor even needed?
	var points []Point
	req, err := influx.NewWriteRequest(context.Background(), points...)
	if err != nil {
		log.Fatalf("%v", err)
	}

	// this is where library user has total freedom to do whatever with request before sending it
	
	// a) send it with ( err := influx.Write(req, [optionalClientsOwnHttpClientInstance]) ) something that combines existing: 
	// service.handleHTTPError(service.DoHTTPRequestWithResponse(req)) logic
	// but does not have mutating state (queues or anything) can be almost considered as value object
	
	// b) or roll your own
	res, err := http.DefaultClient.Do(req)
	if err != nil || (res.StatusCode < 200 || res.StatusCode >= 300) {
		log.Fatalf("%v", err)
	}
}


func (c *Influx) NewWriteRequest(ctx context.Context, point ...write.Point) (*http.Request, error) {
	var body io.Reader
	// ...serialize points ...
	req, err := http.NewRequestWithContext(ctx, "POST", c.apiURL, body)
	// ... add default headers
	// req.Header.Set("Content-Encoding", "gzip")
	// req.Header.Set("Authorization", s.authorization)
	// req.Header.Set("User-Agent", http2.UserAgent)
	return req, err
}

just some remarks

@programmer04
Copy link

One more thing worth note is that the model of Point differs among different packages:

  • influxdb-client-go/v2 (from this package differs a lot from those listed below e.g missing sorting interface, method Key(), etc.)
  • influxdb1-client (probably should be deprecated for version 2.x)
  • influxdb (the database repository, so probably when imported the exact same version of database should be used too to be sure not breaking any compatibility)

To sum up it's not clear which from above should be used for manipulating points with this client and InfluxDB 2.x version

@aldas
Copy link

aldas commented Apr 11, 2021

I think:

  1. too many things are used at pointers. ala
    columns []*FluxColumn

this makes usage of those fields annoying. Passing values is perfectly OK for most of the thing in go. It is hard to understand why it is done so - slice itself is pointer type and having pointers to value makes little sense. Maybe slice to pointer because those types are meant to be modified - which makes little sense.

  1. too many fields are private and awkward to use. If those methods would return value then making those fields publicly accessible would be ok as values are immutable.

type FluxColumn struct {

also by being private fields library has contructor methods like

func NewFluxColumnFull(dataType string, defaultValue string, name string, group bool, index int) *FluxColumn {

witch does not look like idiomatic Go and is annoying to use.

  1. String() implementations like
    func (r *FluxRecord) String() string {

look out of place and not used anywhere. also there exists %#v formating option for fmt package https://golang.org/pkg/fmt/#pkg-overview

func TestFluxColumn_String(t *testing.T) {
	record := NewFluxRecord(1, map[string]interface{}{ "table": 1, "_value": 1.75})
	assert.Equal(t, `&query.FluxRecord{table:1, values:map[string]interface {}{"_value":1.75, "table":1}}`, fmt.Sprintf("%#v", record))
}

also Stringer interface seems to be implemented for structs only in query package so seems like unfinished/abandoned idea.

@ivankudibal
Copy link
Contributor

ivankudibal commented Nov 7, 2022

Release strategy?

  • a'la telegraf (influxdays) 3.0.0, 3.0.1, 3.0.2
  • rc1, rc2?
  • announce availability of branch?

Release plan:

  • Finalize README
  • 2022-11: Release write and query 3.0.0-rc1
  • 2023-01: Release mgmt api, 3.0.0-rc2

@popey , @powersj FYI

@bednar bednar added this to the V3 milestone Oct 9, 2023
@andig
Copy link
Contributor

andig commented Jan 6, 2024

// HTTPClient is used to make API requests.

Not sure if this is still relevant, but unless there are particular reasons for doing to, I would propose to make all user-configurable properties interfaces. In this case, and http.Doer could be appropriate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants