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

Update prometheus/common and add support for --log.format #1658

Merged
merged 3 commits into from
Dec 13, 2018

Conversation

kfdm
Copy link
Contributor

@kfdm kfdm commented Dec 11, 2018

I am not sure if I did this correctly, but I'm willing to accept guidance on how to format things properly. :)

This should handle --log.format for #1657

Signed-off-by: Paul Traylor <paul.traylor@linecorp.com>
Signed-off-by: Paul Traylor <paul.traylor@linecorp.com>
Copy link
Member

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

LGTM. The order of imports in main.go isn't quite right. Although it isn't due to your PR it would be nice to fix it. The packages should be listed in 3 groups, each separated with a blank line:

import (
    // all stdlib packages

    // external packages

    // github.com/prometheus/alertmanager packages
)

@kfdm
Copy link
Contributor Author

kfdm commented Dec 11, 2018

Ok, I'll take care of that tomorrow. :)

Signed-off-by: Paul Traylor <paul.traylor@linecorp.com>
@kfdm
Copy link
Contributor Author

kfdm commented Dec 12, 2018

Assuming that Prometheus is considered an external package from alertmanager's perspective, I believe I have grouped the packages as requested.

@simonpasquier
Copy link
Member

Agreed, the problem exists in the current code base and isn't specific to your change. This group should be split into 2:

"github.com/go-kit/kit/log"
"github.com/go-kit/kit/log/level"
apiv1 "github.com/prometheus/alertmanager/api/v1"
apiv2 "github.com/prometheus/alertmanager/api/v2"
"github.com/prometheus/alertmanager/cluster"
"github.com/prometheus/alertmanager/config"
"github.com/prometheus/alertmanager/dispatch"
"github.com/prometheus/alertmanager/inhibit"
"github.com/prometheus/alertmanager/nflog"
"github.com/prometheus/alertmanager/notify"
"github.com/prometheus/alertmanager/provider/mem"
"github.com/prometheus/alertmanager/silence"
"github.com/prometheus/alertmanager/template"
"github.com/prometheus/alertmanager/types"
"github.com/prometheus/alertmanager/ui"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promhttp"
"github.com/prometheus/common/promlog"
"github.com/prometheus/common/route"
"github.com/prometheus/common/version"
"gopkg.in/alecthomas/kingpin.v2"

@kfdm
Copy link
Contributor Author

kfdm commented Dec 12, 2018

I believe I have done that

"context"
"crypto/md5"
"encoding/binary"
"fmt"
"net"
"net/http"
"net/url"
"os"
"os/signal"
"path/filepath"
"runtime"
"strings"
"sync"
"syscall"
"time"
"github.com/go-kit/kit/log"
"github.com/go-kit/kit/log/level"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promhttp"
"github.com/prometheus/common/promlog"
promlogflag "github.com/prometheus/common/promlog/flag"
"github.com/prometheus/common/route"
"github.com/prometheus/common/version"
"gopkg.in/alecthomas/kingpin.v2"
apiv1 "github.com/prometheus/alertmanager/api/v1"
apiv2 "github.com/prometheus/alertmanager/api/v2"
"github.com/prometheus/alertmanager/cluster"
"github.com/prometheus/alertmanager/config"
"github.com/prometheus/alertmanager/dispatch"
"github.com/prometheus/alertmanager/inhibit"
"github.com/prometheus/alertmanager/nflog"
"github.com/prometheus/alertmanager/notify"
"github.com/prometheus/alertmanager/provider/mem"
"github.com/prometheus/alertmanager/silence"
"github.com/prometheus/alertmanager/template"
"github.com/prometheus/alertmanager/types"
"github.com/prometheus/alertmanager/ui"

though the line with promlogflag "github.com/prometheus/common/promlog/flag",
should that sort by the url part or sort to put it at the bottom (or top to match with api)?

@simonpasquier
Copy link
Member

Ah sorry, I missed the latest update! All good then 👍

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thanks for the patch!

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

Successfully merging this pull request may close these issues.

3 participants